[CAY-909] Notify client job progress #1081

JunhoeKim · 2017-03-28T08:14:58Z

Closed #909
This PR implements a messaging channel between Dolphin driver and client. so Dolphin driver can notify progress information to client in each epoch time.

…er, implement DolphinDriverLauncher

wynot12 · 2017-03-28T08:57:45Z

@JunhoeKim The test has failed. Could you check it?

wynot12 · 2017-03-28T09:02:50Z

I'm attaching error logs of Jenkins.
It looks that you've been working in a stale branch, and it makes your new component incompatible to newly updated changes.

17:56:46 [INFO] -------------------------------------------------------------
17:56:46 [INFO] -------------------------------------------------------------
17:56:46 [ERROR] COMPILATION ERROR : 
17:56:46 [INFO] -------------------------------------------------------------
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[19,40] package edu.snu.cay.dolphin.async.client does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[219,52] cannot find symbol
17:56:46   symbol:   class ProgressMessageHandler
17:56:46   location: class edu.snu.cay.dolphin.async.AsyncDolphinLauncher
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[218,75] package DolphinDriverLauncher does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[217,72] package DolphinDriverLauncher does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[216,75] package DolphinDriverLauncher does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[215,73] package DolphinDriverLauncher does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[214,35] cannot find symbol
17:56:46   symbol:   variable ON_JOB_SUBMITTED
17:56:46   location: class org.apache.reef.client.ClientConfiguration
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[214,75] package DolphinDriverLauncher does not exist
17:56:46 [ERROR] /var/lib/jenkins/workspace/Cay/dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java:[222,37] cannot find symbol

JunhoeKim · 2017-03-28T09:35:35Z

Sorry, I'll check it.

…/cay into notify-client-job-progress

…ent-job-progress

yunseong · 2017-03-30T02:41:57Z

Thanks @JunhoeKim for the PR! As we described in #1084, we need to update REEF at the Jenkins's local repository. Let's handle the issue this afternoon. Thanks!

yunseong · 2017-03-30T09:18:46Z

!rebuild

yunseong · 2017-03-30T09:20:30Z

Sorry, I closed this PR accidently. The build should be successful as I've rebuilt REEF at Jenkins's local repository. Let's see how it goes. :)

wynot12 · 2017-03-30T09:34:30Z

Great :)
Let's update our own development environment, too!

wynot12 · 2017-03-30T09:37:39Z

I'll take a look at the PR tonight :)

wynot12

Overall it looks good :)

I've left several minor comments.
Please take a look!

wynot12 · 2017-03-30T11:26:23Z

common/src/main/java/edu/snu/cay/common/DriverLauncher.java

+import java.util.logging.Logger;
+
+/**
+ * A launcher for Dolphin driver.


This class is not specific for Dolphin.
Could you rewrite the comment?

You may use the comment of REEF's DriverLauncher and specify what you modified.

wynot12 · 2017-03-30T11:30:39Z

common/src/main/java/edu/snu/cay/common/DriverLauncher.java

+    return this.status;
+  }
+
+  /** Update job status and notify the waiting thread. */


Though it's copied from REEF's code, let's make it in the right form by breaking lines.

wynot12 · 2017-03-30T11:30:59Z

common/src/main/java/edu/snu/cay/common/DriverLauncher.java

+  /**
+  * Handler of {@link JobMessage} from driver.
+  * It logs progress report to console.
+  */


Please fix the indentation.

wynot12 · 2017-03-30T11:39:44Z

common/src/main/java/edu/snu/cay/common/DriverLauncher.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package edu.snu.cay.common;


Let's put it into edu.snu.cay.common.client package.

wynot12 · 2017-03-30T11:41:49Z

dolphin/async/src/main/java/edu/snu/cay/dolphin/async/AsyncDolphinLauncher.java

@@ -143,7 +142,6 @@ public static LauncherStatus launch(final String jobName,
              basicParameterInjector.getNamedInstance(LocalRuntimeMaxNumEvaluators.class),
              basicParameterInjector.getNamedInstance(JVMHeapSlack.class)) :
          getYarnRuntimeConfiguration(basicParameterInjector.getNamedInstance(JVMHeapSlack.class));
-


Could you undo this change, which is irrelevant from the PR?

wynot12 · 2017-03-30T11:46:43Z

services/ps/src/main/java/edu/snu/cay/services/ps/driver/impl/ClockManager.java

@@ -72,6 +72,11 @@
   */
  private int globalMinimumClock;

+  /**
+   * The list of EventHandler about progress update.


The list of listeners for the update of {@link #globalMinimumClock}.

wynot12 · 2017-03-30T11:47:51Z

services/ps/src/main/java/edu/snu/cay/services/ps/driver/impl/ClockManager.java

+  /**
+   * The list of EventHandler about progress update.
+   */
+  private final List<EventHandler<Integer>> progressUpdateCallbacks = new ArrayList<>();


Better to rename it as clockUpdateListeners.

wynot12 · 2017-03-30T11:49:17Z

services/ps/src/main/java/edu/snu/cay/services/ps/driver/impl/ClockManager.java

+   * Add listener to progress update list.
+   * @param callback when #globalMinimumClock increases, callback functions in list are called.
+   */
+  public void addProgressUpdateListener(final EventHandler<Integer> callback) {


Let's rename it as addClockUpdateListener.

JunhoeKim · 2017-04-03T08:32:44Z

Thank you, I'll check and resolve them.

…ent-job-progress

…/cay into notify-client-job-progress

…u/cay into notify-client-job-progress

wynot12 · 2017-04-04T02:34:09Z

@JunhoeKim Is it ready for review?
Then I'll take a final look in this afternoon.

wynot12 · 2017-04-04T09:16:20Z

LGTM. I'll merge it once test pass.

wynot12 · 2017-04-04T10:51:47Z

It looks that Jenkins uses jar files of PR branches.
Let's fix this build environment issue later.

wynot12 and others added 5 commits March 27, 2017 16:29

commit

c7d3688

DolphinDriverLauncher in AsyncDolphinDriver and AsyncDolphinLauncher

ca8427c

[CAY-909] Update AsyncDolphinDriver, AsyncDolphinLauncher, ClockManag…

8ea7311

…er, implement DolphinDriverLauncher

devide ProgressMessageHandler from Dolphin specific driver launcher

59e7b0a

Merge branch 'master' into notify-client-job-progress

f73d6d9

kimjunhoe added 4 commits March 28, 2017 18:38

Add DolphintDriverLauncher class

6251a9f

Merge branch 'notify-client-job-progress' of http://github.com/cmssnu…

5f8a779

…/cay into notify-client-job-progress

Merge branch 'master' of http://github.com/cmssnu/cay into notify-cli…

233af49

…ent-job-progress

Change redundant class in DolphinDriverLauncher

3d8ef07

wynot12 requested review from sj6077 and wynot12 and removed request for sj6077 March 29, 2017 05:23

wynot12 and others added 3 commits March 29, 2017 14:58

in progress

7255025

set StringJobMessageHandler in DriverLauncher

150a856

Merge branch 'master' into notify-client-job-progress

5401742

yunseong closed this Mar 30, 2017

yunseong reopened this Mar 30, 2017

wynot12 mentioned this pull request Mar 30, 2017

Version problem in Jenkins REEF #1084

Open

wynot12 suggested changes Mar 30, 2017

View reviewed changes

kimjunhoe added 2 commits April 3, 2017 17:41

formats, name of listener in ClockManager modified

e3359f7

Merge branch 'master' of http://github.com/cmssnu/cay into notify-cli…

6815a45

…ent-job-progress

kimjunhoe and others added 4 commits April 3, 2017 17:49

Merge branch 'notify-client-job-progress' of http://github.com/cmssnu…

76e02c6

…/cay into notify-client-job-progress

Merge branch 'master' into notify-client-job-progress

2df8d37

undo delete blank line

d4d4bf3

XMerge branch 'notify-client-job-progress' of http://github.com/cmssn…

c4b2a06

…u/cay into notify-client-job-progress

Minor update

6e0e9aa

wynot12 force-pushed the notify-client-job-progress branch from 3513280 to 6e0e9aa Compare April 4, 2017 09:14

wynot12 approved these changes Apr 4, 2017

View reviewed changes

Merge branch 'master' into notify-client-job-progress

d4e5e28

wynot12 merged commit 89eb3c2 into master Apr 5, 2017

wynot12 deleted the notify-client-job-progress branch April 5, 2017 13:05

wynot12 mentioned this pull request Apr 27, 2017

[CAY-1109] Start workers with minimum epoch idx reported by running workers #1124

Merged

wynot12 mentioned this pull request Jul 14, 2017

[MINOR] Use default DriverLauncher #1205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CAY-909] Notify client job progress #1081

[CAY-909] Notify client job progress #1081

JunhoeKim commented Mar 28, 2017

wynot12 commented Mar 28, 2017

wynot12 commented Mar 28, 2017

JunhoeKim commented Mar 28, 2017

yunseong commented Mar 30, 2017

yunseong commented Mar 30, 2017

yunseong commented Mar 30, 2017

wynot12 commented Mar 30, 2017

wynot12 commented Mar 30, 2017 •

edited

Loading

wynot12 left a comment

wynot12 Mar 30, 2017 •

edited

Loading

wynot12 Mar 30, 2017

wynot12 Mar 30, 2017

wynot12 Mar 30, 2017

wynot12 Mar 30, 2017 •

edited

Loading

wynot12 Mar 30, 2017

wynot12 Mar 30, 2017

wynot12 Mar 30, 2017

JunhoeKim commented Apr 3, 2017

wynot12 commented Apr 4, 2017

wynot12 commented Apr 4, 2017

wynot12 commented Apr 4, 2017 •

edited

Loading

[CAY-909] Notify client job progress #1081

[CAY-909] Notify client job progress #1081

Conversation

JunhoeKim commented Mar 28, 2017

wynot12 commented Mar 28, 2017

wynot12 commented Mar 28, 2017

JunhoeKim commented Mar 28, 2017

yunseong commented Mar 30, 2017

yunseong commented Mar 30, 2017

yunseong commented Mar 30, 2017

wynot12 commented Mar 30, 2017

wynot12 commented Mar 30, 2017 • edited Loading

wynot12 left a comment

Choose a reason for hiding this comment

wynot12 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

wynot12 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

wynot12 Mar 30, 2017

Choose a reason for hiding this comment

JunhoeKim commented Apr 3, 2017

wynot12 commented Apr 4, 2017

wynot12 commented Apr 4, 2017

wynot12 commented Apr 4, 2017 • edited Loading

wynot12 commented Mar 30, 2017 •

edited

Loading

wynot12 Mar 30, 2017 •

edited

Loading

wynot12 Mar 30, 2017 •

edited

Loading

wynot12 commented Apr 4, 2017 •

edited

Loading