You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "Peiyingy (via GitHub)" <gi...@apache.org> on 2023/06/21 17:24:40 UTC

[GitHub] [gobblin] Peiyingy opened a new pull request, #3708: [GOBBLIN-1841] Move disabling of current live instances to the GobblinClusterManager startup

Peiyingy opened a new pull request, #3708:
URL: https://github.com/apache/gobblin/pull/3708

   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-1841] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1841
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   
   The method `disableLiveHelixInstances()` originally exists in `GobblinYarnAppLauncher` to disable pre-existing live instances in a Helix cluster. However, there are situations where these orphaned instances still exist while the disable method is not triggered at the Yarn level. 
   Thus, we moved this method to `GobblinClusterManager`, and invokes it at the start stage, to guarantee that each time these pre-existing live instances can be disabled.
   
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   <img width="518" alt="Screenshot 2023-06-21 at 10 21 26 AM" src="https://github.com/apache/gobblin/assets/112960226/6d7468ce-8826-4984-8d2b-7812e503e02d">
   
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


-- 
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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1242970851


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinApplicationMaster.java:
##########
@@ -135,6 +141,26 @@ protected MultiTypeMessageHandlerFactory getUserDefinedMessageHandlerFactory() {
     return new ControllerUserDefinedMessageHandlerFactory();
   }
 
+  @Override
+  public synchronized void setupHelix() {
+    super.setupHelix();
+    this.disableTaskRunnersFromPreviousExecutions(this.multiManager);
+  }
+
+  public static void disableTaskRunnersFromPreviousExecutions(GobblinHelixMultiManager multiManager) {
+    HelixManager helixManager = multiManager.getJobClusterHelixManager();
+    HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+    String clusterName = helixManager.getClusterName();
+    HelixAdmin helixAdmin = helixManager.getClusterManagmentTool();
+    Set<String> taskRunners = HelixUtils.getParticipants(helixDataAccessor,
+        GobblinYarnTaskRunner.HELIX_YARN_INSTANCE_NAME_PREFIX);
+    LOGGER.warn("Found {} task runners in the cluster.", taskRunners.size());
+    for (String taskRunner: taskRunners) {

Review Comment:
   whitespace before the `:`. But this will probably be caught by the linter



-- 
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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1242971589


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinApplicationMaster.java:
##########
@@ -135,6 +141,26 @@ protected MultiTypeMessageHandlerFactory getUserDefinedMessageHandlerFactory() {
     return new ControllerUserDefinedMessageHandlerFactory();
   }
 
+  @Override
+  public synchronized void setupHelix() {
+    super.setupHelix();
+    this.disableTaskRunnersFromPreviousExecutions(this.multiManager);
+  }
+
+  public static void disableTaskRunnersFromPreviousExecutions(GobblinHelixMultiManager multiManager) {
+    HelixManager helixManager = multiManager.getJobClusterHelixManager();
+    HelixDataAccessor helixDataAccessor = helixManager.getHelixDataAccessor();
+    String clusterName = helixManager.getClusterName();
+    HelixAdmin helixAdmin = helixManager.getClusterManagmentTool();
+    Set<String> taskRunners = HelixUtils.getParticipants(helixDataAccessor,
+        GobblinYarnTaskRunner.HELIX_YARN_INSTANCE_NAME_PREFIX);
+    LOGGER.warn("Found {} task runners in the cluster.", taskRunners.size());
+    for (String taskRunner: taskRunners) {

Review Comment:
   whitespace before the `:`. But this will probably be caught by the linter



-- 
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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1239159140


##########
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:
   Upon further investigation, this job cluster manager should be connected elsewhere. But I see another issue where you are fetching all live instances (include itself!). Which means that when the AM calls this method, it will disable itself.
   
   Please take a look at the below code https://github.com/apache/gobblin/blob/5af6bca57df909e44b995e5b2d667c70e0399877/gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnAutoScalingManager.java#L187C1-L196C6
   
   This code fetches all live participants that are task runners. We should only disable taskrunners, so let's do that instead.
   
   And then we can also rename this method to `disableTaskRunnersFromPreviousExecutions`. 



-- 
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

[GitHub] [gobblin] ZihanLi58 merged pull request #3708: [GOBBLIN-1841] Move disabling of current live instances to the GobblinClusterManager startup

Posted by "ZihanLi58 (via GitHub)" <gi...@apache.org>.
ZihanLi58 merged PR #3708:
URL: https://github.com/apache/gobblin/pull/3708


-- 
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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1248173524


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java:
##########
@@ -540,26 +537,6 @@ void connectHelixManager() {
     }
   }
 
-  /**
-   * A method to disable pre-existing live instances in a Helix cluster. This can happen when a previous Yarn application
-   * leaves behind orphaned Yarn worker processes. Since Helix does not provide an API to drop a live instance, we use
-   * the disable instance API to fence off these orphaned instances and prevent them from becoming participants in the
-   * new cluster.
-   *
-   * NOTE: this is a workaround for an existing YARN bug. Once YARN has a fix to guarantee container kills on application
-   * completion, this method should be removed.
-   */

Review Comment:
   Can you add this comment back to the new impl



-- 
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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@gobblin.apache.org

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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1239159140


##########
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:
   Upon further investigation, this job cluster manager should be connected elsewhere. But I see another issue where you are fetching all live instances (include itself!). Which means that when the AM calls this method, it will disable itself.
   
   Please take a look at the below code https://github.com/apache/gobblin/blob/5af6bca57df909e44b995e5b2d667c70e0399877/gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnAutoScalingManager.java#L187C1-L196C6
   
   This code fetches all live participants that are task runners. So we should only disable those instances.
   
   And then we can also rename this method to `disableTaskRunnersFromPreviousExecutions`. 



-- 
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

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

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#issuecomment-1604765355

   ## [Codecov](https://app.codecov.io/gh/apache/gobblin/pull/3708?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#3708](https://app.codecov.io/gh/apache/gobblin/pull/3708?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (759fa70) into [master](https://app.codecov.io/gh/apache/gobblin/commit/51a852d506b749b9ac33568aff47105e14972a57?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (51a852d) will **decrease** coverage by `2.14%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3708      +/-   ##
   ============================================
   - Coverage     46.97%   44.83%   -2.14%     
   + Complexity    10794     2102    -8692     
   ============================================
     Files          2138      411    -1727     
     Lines         84132    17746   -66386     
     Branches       9356     2163    -7193     
   ============================================
   - Hits          39518     7956   -31562     
   + Misses        41015     8930   -32085     
   + Partials       3599      860    -2739     
   ```
   
   
   | [Impacted Files](https://app.codecov.io/gh/apache/gobblin/pull/3708?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [.../apache/gobblin/cluster/GobblinClusterManager.java](https://app.codecov.io/gh/apache/gobblin/pull/3708?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJNYW5hZ2VyLmphdmE=) | `57.52% <100.00%> (+4.25%)` | :arrow_up: |
   
   ... and [1736 files with indirect coverage changes](https://app.codecov.io/gh/apache/gobblin/pull/3708/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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

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

Posted by "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

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

Posted by "homatthew (via GitHub)" <gi...@apache.org>.
homatthew commented on code in PR #3708:
URL: https://github.com/apache/gobblin/pull/3708#discussion_r1237392824


##########
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 or alternatively you can move this method to a yarn specific class



-- 
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