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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
