-
Notifications
You must be signed in to change notification settings - Fork 746
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
encapsulate-non-helix-job-launch-logic #2193
encapsulate-non-helix-job-launch-logic #2193
Conversation
Depends on #2191 |
@HappyRay Seems like there are code conflicts, please resolve them. |
Put the logic in its own class. Testing: The integration test org.apache.gobblin.cluster.ClusterIntegrationTest passed.
4193941
to
92c2313
Compare
Resolved. |
Here is the test failure. Looks like a flaky test to me. Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. STARTED Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. FAILED Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. STARTED Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. FAILED Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. STARTED Gradle suite > Gradle test > org.apache.gobblin.rest.JobExecutionInfoServerTest. FAILED 3 tests completed, 3 failed FAILURE: Build failed with an exception.
BUILD FAILED Total time: 15 mins 36.995 secs idl compatibility report:
[RS-I]:Resource for "/home/travis/build/apache/incubator-gobblin/gobblin-rest-service/gobblin-rest-api/src/main/snapshot/org.apache.gobblin.rest.jobExecutions.snapshot.json" is not found. This endpoint will not be released. Please remove this file and build again travis_time:end:0c852a20:start=1512748975620375525,finish=1512749913137284912,duration=937516909387 travis_time:end:17255fcd:start=1512749913147336308,finish=1512749913156772809,duration=9436501 �[0m travis_time:end:22b4ecd7:start=1512749913182234549,finish=1512749942560795801,duration=29378561252
|
All three tests passed locally. |
@htran1 Please review, this might impact your work with MySQL store usage. |
SharedResourcesBroker<GobblinScopeTypes> globalBroker = null; | ||
try { | ||
return runInternal(globalBroker); | ||
}finally { |
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.
missing a space
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.
done
} | ||
} | ||
|
||
private int runInternal(SharedResourcesBroker<GobblinScopeTypes> globalBroker) |
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.
Can you put docs for the return param?
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.
done
if (workUnit instanceof MultiWorkUnit) { | ||
// Flatten the MultiWorkUnit so the job configuration properties can be added to each individual WorkUnits | ||
List<WorkUnit> flattenedWorkUnits = JobLauncherUtils.flattenWorkUnits(((MultiWorkUnit) workUnit).getWorkUnits()); | ||
workUnits.addAll(flattenedWorkUnits); |
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.
Is it possible to simplify this? Maybe return flattenedWorkunits directly in if condition? In else condition just return ImmutableList.of(workunit)?
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.
Maybe,
I just moved the existing code. I would prefer not to make this change for this PR.
this.stateStores = stateStores; | ||
} | ||
|
||
public int run() |
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.
Please add java docs for the return type.
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.
done.
return workUnitSize; | ||
|
||
} | ||
private void closeGlobalBroker(SharedResourcesBroker<GobblinScopeTypes> globalBroker) { |
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.
Missing an empty line.
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.
done
} | ||
|
||
private List<WorkUnit> getWorkUnits() | ||
throws IOException { |
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.
Is this a coding style that exceptions will be a new line?
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.
IDE auto formatted it based on the style config Gobblin provides.
*/ | ||
@Alpha | ||
public class GobblinHelixTask implements Task { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(GobblinHelixTask.class); | ||
private static final Logger _logger = LoggerFactory.getLogger(GobblinHelixTask.class); |
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 the underscore be removed for code consistency?
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.
See comment above.
} | ||
} | ||
_logger.error("GobblinHelixTask failed due to " + t.getMessage(), t); | ||
return new TaskResult(TaskResult.Status.FAILED, Throwables.getStackTraceAsString(t)); |
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.
How come the state is changed to FAILED? ERROR means helix may retry.
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.
ERROR has been deprecated and replaced with FAILED.
see the Helix doc for details.
*/ | ||
@Alpha | ||
public class GobblinHelixTask implements Task { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(GobblinHelixTask.class); | ||
private static final Logger _logger = LoggerFactory.getLogger(GobblinHelixTask.class); |
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.
Why the change here? We usually avoid mixing '_' and this.
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.
The existing code doesn't use this consistently anyway.
This change is made to make logger usage more consistent and fit the google coding style and Linkedin coding style.
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.
Replaced all the fields with _ prefix and removed all usages of this.
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.
I don't see a consistent usage of this.logger elsewhere if the convention is not to mix _ with this. Did I miss something?
Also inlined a method. The old code has a bug: the globalBroker variable will stay null. Changed the implementation to use try with statement.
+1 LGTM |
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.
+1
Opened an issue to track the flaky test. |
task execution logic Put the logic in its own class. Also changed: * Use a try-with statement to close the global broker. * Fix a Helix warning: ERROR is replaced with FAILED. Testing: The integration test org.apache.gobblin.cluster.ClusterIntegrationTest passed. Also inlined a method. The old code has a bug: the globalBroker variable will stay null. Closes apache#2193 from HappyRay/encapsulate-non-helix- job-launch-logic
task execution logic Put the logic in its own class. Also changed: * Use a try-with statement to close the global broker. * Fix a Helix warning: ERROR is replaced with FAILED. Testing: The integration test org.apache.gobblin.cluster.ClusterIntegrationTest passed. Also inlined a method. The old code has a bug: the globalBroker variable will stay null. Closes apache#2193 from HappyRay/encapsulate-non-helix- job-launch-logic
@HappyRay How were you able to resolve the following failure: Incompatible changes:
Resource for "/home/travis/build/apache/incubator-gobblin/gobblin-rest-service/gobblin-rest-api/src/main/snapshot/org.apache.gobblin.rest.jobExecutions.snapshot.json" is not found. This endpoint will not be released. Please remove this file and build again
Resource for "/home/travis/build/apache/incubator-gobblin/gobblin-rest-service/gobblin-rest-api/src/main/idl/org.apache.gobblin.rest.jobExecutions.restspec.json" is not found. This endpoint will not be released. Please remove this file and build again``` |
[GOBBLIN-336] Encapsulate the non-Helix specific task execution logic
Put the logic in its own class.
Also changed:
Testing:
The integration test org.apache.gobblin.cluster.ClusterIntegrationTest
passed.