New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GOBBLIN-1107: Lazily initialize Helix TaskStateModelFactory in Gobbli… #2947
Conversation
@autumnust please review. |
Codecov Report
@@ Coverage Diff @@
## master #2947 +/- ##
============================================
- Coverage 45.56% 44.64% -0.93%
+ Complexity 9153 8988 -165
============================================
Files 1936 1936
Lines 73292 73295 +3
Branches 8088 8088
============================================
- Hits 33395 32722 -673
- Misses 36800 37521 +721
+ Partials 3097 3052 -45 Continue to review full report at Codecov.
|
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java
Show resolved
Hide resolved
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java
Outdated
Show resolved
Hide resolved
gobblin-cluster/src/test/java/org/apache/gobblin/cluster/GobblinTaskRunnerTest.java
Outdated
Show resolved
Hide resolved
@@ -156,13 +179,73 @@ public void testConnectHelixManagerWithRetry() { | |||
//Ensure that connect with retry succeeds | |||
corruptGobblinTaskRunner.connectHelixManagerWithRetry(); | |||
Assert.assertTrue(true); | |||
|
|||
corruptGobblinTaskRunner.disconnectHelixManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should corruptGobblinTaskRunner
be a local variable?
gobblin-cluster/src/test/java/org/apache/gobblin/cluster/GobblinTaskRunnerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, LGTM if addressed.
/** | ||
* A helper method that creates a partial instance structure in ZK. | ||
*/ | ||
private static void createPartialInstanceStructure(HelixManager helixManager, String zkConnectString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be moved into utils class newly created ?
+1 LGTM. |
Closes apache#2947 from sv2000/taskRunner
Closes apache#2947 from sv2000/taskRunner
…nTaskRunner
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
Description
Current behavior eagerly initializes the taskStateModelFactory inside the constructor. This causes Helix task assignment to be stuck when a Helix manager connection needs to be retried.
Tests
Existing unit tests.
Commits