[GitHub] [gobblin] homatthew commented on a diff in pull request #3708: [GOBBLIN-1841] Move disabling of current live instances to the GobblinClusterManager startup

["homatthew (via GitHub)" <gi...@apache.org>]

homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1237372963


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java:
##########
@@ -504,6 +506,18 @@ private static void printUsage(Options options) {
     formatter.printHelp(GobblinClusterManager.class.getSimpleName(), options);
   }
 
+  public void disableLiveHelixInstances() {
+    HelixManager helixManager = this.multiManager.getJobClusterHelixManager();

Review Comment:
   Do we need to check that this is actually connected before using it? If you check the multimanager, it only connects if we are in dedicated mode. 
   
   So would this throw an exception? 



##########
gobblin-cluster/src/test/java/org/apache/gobblin/cluster/GobblinClusterManagerTest.java:
##########
@@ -208,4 +221,51 @@ public void assertMessageReception(Message message) {
     Assert.assertEquals(message.getMsgType(), GobblinHelixConstants.SHUTDOWN_MESSAGE_TYPE);
     Assert.assertEquals(message.getMsgSubType(), HelixMessageSubTypes.WORK_UNIT_RUNNER_SHUTDOWN.toString());
   }
+
+  @Test
+  public void testDisableLiveHelixInstances() throws Exception {
+    GobblinHelixMultiManager mockMultiManager = Mockito.mock(GobblinHelixMultiManager.class);
+
+    TestGobblinClusterManager testGobblinClusterManager = new TestGobblinClusterManager(GobblinClusterManagerTest.class.getSimpleName(), TestHelper.TEST_APPLICATION_ID, config,
+            Optional.<Path>absent(), mockMultiManager);
+
+    HelixManager mockHelixManager = Mockito.mock(HelixManager.class);
+    when(mockMultiManager.getJobClusterHelixManager()).thenReturn(mockHelixManager);
+
+    HelixAdmin mockHelixAdmin = Mockito.mock(HelixAdmin.class);
+    when(mockHelixManager.getClusterManagmentTool()).thenReturn(mockHelixAdmin);
+    when(mockHelixManager.getClusterName()).thenReturn("mockCluster");
+
+    HelixDataAccessor mockAccessor = Mockito.mock(HelixDataAccessor.class);
+    when(mockHelixManager.getHelixDataAccessor()).thenReturn(mockAccessor);
+
+    PropertyKey.Builder mockBuilder = Mockito.mock(PropertyKey.Builder.class);
+    when(mockAccessor.keyBuilder()).thenReturn(mockBuilder);
+
+    PropertyKey mockLiveInstancesKey = Mockito.mock(PropertyKey.class);
+    when(mockBuilder.liveInstances()).thenReturn(mockLiveInstancesKey);
+
+    List<String> mockLiveInstances = Arrays.asList("TestInstance_0");
+    when(mockAccessor.getChildNames(mockLiveInstancesKey)).thenReturn(mockLiveInstances);
+
+    ConfigAccessor mockConfigAccessor = Mockito.mock(ConfigAccessor.class);
+    when(mockHelixManager.getConfigAccessor()).thenReturn(mockConfigAccessor);
+
+    ClusterConfig mockClusterConfig = Mockito.mock(ClusterConfig.class);
+    when(mockConfigAccessor.getClusterConfig("GobblinClusterManagerTest")).thenReturn(mockClusterConfig);
+
+    testGobblinClusterManager.start();
+
+    Mockito.verify(mockHelixAdmin).enableInstance("mockCluster", "TestInstance_0", false);
+  }
+
+  public class TestGobblinClusterManager extends GobblinClusterManager {

Review Comment:
   Nit: Seems like this replaces the already instantiated multi manager with a new injected one. Note that the GobblinClusterManager calls the init method in the constructor and anyone who modifies it will need to duplicate their logic across the test class you created and the actual impl.
    
   ```
       initializeHelixManager();
   ```
   
   Since we are going down the path of extending the ClusterManager for dependency injection, I recommend a slightly different approach:
   1. Extract the actual instantiation of the helixManager to a separate method e.g. `getHelixManager` and use this new method in the `initializeHelixManager` method. 
   2. Override the instantiation method  `getHelixManager` in the test class `TestGobblinClusterManager` so that it returns a mock 
   
   Since this test cluster manager is a non static class, it should have access to any members of the outer parent class `GobblinClusterManagerTest`. You can take advantage of this to access the mocked from within this `TestClusterManager`.
   
   The advantage to this approach is that if anyone modifies the initializeHelixManager, they won't need to think about duplicating the logic in your test class. 



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java:
##########
@@ -280,6 +281,7 @@ public synchronized void start() {
 
     this.eventBus.register(this);
     this.multiManager.connect();
+    disableLiveHelixInstances();

Review Comment:
   Seems like this should only be enabled if we are non standalone mode



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java:
##########
@@ -280,6 +281,7 @@ public synchronized void start() {
 
     this.eventBus.register(this);
     this.multiManager.connect();

Review Comment:
   The multimanager only connects the job cluster helix manager if we are in dedicated mode. This class is also shared between YARN and non-YARN apps. 
   
   But we are moving the previous logic from a yarn specific class. Maybe it makes more sense to have the disableLiveHelixInstances method as part of the GobblinApplicationMaster which extends this class but is yarn specific



##########
gobblin-cluster/src/test/java/org/apache/gobblin/cluster/GobblinClusterManagerTest.java:
##########
@@ -208,4 +221,51 @@ public void assertMessageReception(Message message) {
     Assert.assertEquals(message.getMsgType(), GobblinHelixConstants.SHUTDOWN_MESSAGE_TYPE);
     Assert.assertEquals(message.getMsgSubType(), HelixMessageSubTypes.WORK_UNIT_RUNNER_SHUTDOWN.toString());
   }
+
+  @Test
+  public void testDisableLiveHelixInstances() throws Exception {
+    GobblinHelixMultiManager mockMultiManager = Mockito.mock(GobblinHelixMultiManager.class);
+
+    TestGobblinClusterManager testGobblinClusterManager = new TestGobblinClusterManager(GobblinClusterManagerTest.class.getSimpleName(), TestHelper.TEST_APPLICATION_ID, config,
+            Optional.<Path>absent(), mockMultiManager);
+
+    HelixManager mockHelixManager = Mockito.mock(HelixManager.class);
+    when(mockMultiManager.getJobClusterHelixManager()).thenReturn(mockHelixManager);
+
+    HelixAdmin mockHelixAdmin = Mockito.mock(HelixAdmin.class);
+    when(mockHelixManager.getClusterManagmentTool()).thenReturn(mockHelixAdmin);
+    when(mockHelixManager.getClusterName()).thenReturn("mockCluster");
+
+    HelixDataAccessor mockAccessor = Mockito.mock(HelixDataAccessor.class);
+    when(mockHelixManager.getHelixDataAccessor()).thenReturn(mockAccessor);
+
+    PropertyKey.Builder mockBuilder = Mockito.mock(PropertyKey.Builder.class);
+    when(mockAccessor.keyBuilder()).thenReturn(mockBuilder);
+
+    PropertyKey mockLiveInstancesKey = Mockito.mock(PropertyKey.class);
+    when(mockBuilder.liveInstances()).thenReturn(mockLiveInstancesKey);
+
+    List<String> mockLiveInstances = Arrays.asList("TestInstance_0");

Review Comment:
   Let's test disabling more than once instance. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org