Introduce Workerfactory #188

halakaraki · 2018-07-18T22:58:01Z

No description provided.

mfateev · 2018-07-19T00:07:58Z

src/main/java/com/uber/cadence/common/Validate.java

+
+package com.uber.cadence.common;
+
+public class Validate {


Use Preconditions from Guava library: https://github.com/google/guava/wiki/PreconditionsExplained
https://google.github.io/guava/releases/21.0/api/docs/com/google/common/base/Preconditions.html

mfateev · 2018-07-19T00:10:36Z

src/main/java/com/uber/cadence/worker/Worker.java

+
+    private final ArrayList<Worker> workers = new ArrayList<>();
+    private final Supplier<IWorkflowService> getWorkFlowService;
+    private String domain;


mfateev · 2018-07-19T00:12:16Z

src/main/java/com/uber/cadence/worker/Worker.java

+      this(() -> new WorkflowServiceTChannel(host, port), domain);
+    }
+
+    public Factory(Supplier<IWorkflowService> getWorkFlowService, String domain) {


What's the use case for passing Supplier instead of the IWorkflowService instance?

Control over whether we want to pass a single instance or an instance per worker.

What's the use case to have an instance per worker?

Nope, changed this to passing in an instance. The factory will not be responsible for closing the instance passed in.

mfateev · 2018-07-19T00:13:11Z

src/main/java/com/uber/cadence/worker/Worker.java

+
+  public static final class Factory {
+
+    private final ArrayList<Worker> workers = new ArrayList<>();


Should be
private final List<Worker> workers = new ArrayList<>();

mfateev · 2018-07-19T00:13:33Z

src/main/java/com/uber/cadence/worker/Worker.java

+      }
+
+      Worker worker = new Worker(getWorkFlowService.get(), domain, taskList, options);
+      workers.add(worker);


Access to workers is not sycnrhonized

mfateev · 2018-07-19T00:14:11Z

src/main/java/com/uber/cadence/worker/Worker.java

+        state = State.Started;
+      }
+
+      for (Worker worker : workers) {


Access not synchronized

mfateev · 2018-07-19T00:15:19Z

src/main/java/com/uber/cadence/worker/Worker.java

+      }
+    }
+
+    enum State {


private enum

mfateev · 2018-07-19T00:16:36Z

src/test/java/com/uber/cadence/workerFactory/workerFactoryTests.java

+import java.time.Duration;
+import org.junit.Test;
+
+public class workerFactoryTests {


mfateev · 2018-07-19T00:17:16Z

src/test/java/com/uber/cadence/workerFactory/workerFactoryTests.java

+public class workerFactoryTests {
+
+  @Test
+  public void WhenAFactoryIsStartedAllWorkersStart() {


Method naming convention is to start from a lower case letter:
whenAFactoryIsStartedAllWorkersStart

mfateev · 2018-07-24T22:45:45Z

build.gradle

@@ -50,6 +50,7 @@ dependencies {
    compile group: 'org.apache.thrift', name: 'libthrift', version: '0.9.3'
    compile group: 'com.google.code.gson', name: 'gson', version: '2.8.2'
    compile group: 'com.uber.m3', name: 'tally-core', version: '0.2.1'
+    compile 'com.google.guava:guava:25.1-jre'


Could you use the same format as other "compile" entries for consistency?

mfateev · 2018-07-24T23:27:06Z

src/main/java/com/uber/cadence/worker/Worker.java

-    this(new WorkflowServiceTChannel(), domain, taskList, options);
-  }
+  private Worker(IWorkflowService service, String domain, String taskList, WorkerOptions options) {
+    Preconditions.checkNotNull(service, "service should not be null");


The Java 8 introduced Objects.requireNotNull for this purpose:

https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html

mfateev · 2018-07-24T23:27:46Z

src/main/java/com/uber/cadence/worker/Worker.java

+  private Worker(IWorkflowService service, String domain, String taskList, WorkerOptions options) {
+    Preconditions.checkNotNull(service, "service should not be null");
+    Preconditions.checkArgument(
+        domain != null && !"".equals(domain), "domain should not be an empty string");


Use https://google.github.io/guava/releases/23.0/api/docs/com/google/common/base/Strings.html#isNullOrEmpty-java.lang.String-

halakaraki added 3 commits July 17, 2018 17:23

Worker Factory Compiles

5449cf8

tests pass

af27ef3

factory tests pass

82b8161

halakaraki requested review from mfateev and meiliang86 July 18, 2018 22:58

mfateev suggested changes Jul 19, 2018

View reviewed changes

halakaraki added 8 commits July 19, 2018 10:22

use preconditions library

9371563

address CR comments

3797daa

use preconditions checks in Worker.java

cce69a5

use gauva null coelescer

dbabd26

tests pass again

c91c94d

Merge branch 'master' into workerfactory

891461d

merge with master

b8858ac

address pr comments

85722b1

mfateev approved these changes Jul 24, 2018

View reviewed changes

halakaraki added 2 commits July 26, 2018 10:41

resolve PR comments

586e609

resolve PR comments

3e2d86a

halakaraki merged commit 9beadba into cadence-workflow:master Jul 26, 2018


		public static final class Factory {

		private final ArrayList<Worker> workers = new ArrayList<>();

Introduce Workerfactory #188

Introduce Workerfactory #188

Uh oh!

Conversation

halakaraki commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!