homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1242958291
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java:
##########
@@ -504,6 +516,7 @@ private static void printUsage(Options options) {
formatter.printHelp(GobblinClusterManager.class.getSimpleName(), options);
}
+
Review Comment:
Remove random whitespace. This will probably fail the linter
##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnAutoScalingManager.java:
##########
@@ -272,7 +260,7 @@ void runInternal() {
}
// Find all participants appearing in this cluster. Note that Helix
instances can contain cluster-manager
// and potentially replanner-instance.
- Set<String> allParticipants =
getParticipants(HELIX_YARN_INSTANCE_NAME_PREFIX);
+ Set<String> allParticipants =
HelixUtils.getParticipants(helixDataAccessor,HELIX_YARN_INSTANCE_NAME_PREFIX);
Review Comment:
Add whitespace after the comma. This will probably fail the linter
##########
gobblin-yarn/src/test/java/org/apache/gobblin/yarn/GobblinApplicationMasterTest.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.gobblin.yarn;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixManager;
+import org.apache.helix.HelixProperty;
+import org.apache.helix.PropertyKey;
+import org.apache.helix.model.ClusterConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.Test;
+
+import junit.framework.TestCase;
+
+import org.apache.gobblin.cluster.GobblinHelixMultiManager;
+
+import static org.mockito.Mockito.when;
+
+
+public class GobblinApplicationMasterTest extends TestCase {
+ @Test
+ public void testDisableTaskRunnersFromPreviousExecutions() {
+ GobblinHelixMultiManager mockMultiManager =
Mockito.mock(GobblinHelixMultiManager.class);
+
+ 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);
+
+ int instanceCount = 3;
+ Map<String, HelixProperty> mockChildValuesMap = new HashMap<>();
+ for (int i = 0; i < instanceCount; i++) {
+ mockChildValuesMap.put("GobblinYarnTaskRunner_TestInstance_" + i,
Mockito.mock(HelixProperty.class));
Review Comment:
We should also be adding non `GobblinYarnTaskRunner`s as part of the
cluster. E.g. `GobblinClusterManager`. Because we'd only want to disable those
that start with `GobblinYarnTaskRunner`.
And then in your mockito verify, you should verify we never call disable on
`GobblinClusterManager`
--
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]
