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.
HappyRay
force-pushed
the
encapsulate-non-helix-job-launch-logic
branch
from
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 { |
| } | ||
| } | ||
|
|
||
| private int runInternal(SharedResourcesBroker<GobblinScopeTypes> globalBroker) |
There was a problem hiding this comment.
Can you put docs for the return param?
| 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.
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.
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.
Please add java docs for the return type.
| return workUnitSize; | ||
|
|
||
| } | ||
| private void closeGlobalBroker(SharedResourcesBroker<GobblinScopeTypes> globalBroker) { |
| } | ||
|
|
||
| private List<WorkUnit> getWorkUnits() | ||
| throws IOException { |
There was a problem hiding this comment.
Is this a coding style that exceptions will be a new line?
There was a problem hiding this comment.
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.
Should the underscore be removed for code consistency?
| } | ||
| } | ||
| _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.
How come the state is changed to FAILED? ERROR means helix may retry.
There was a problem hiding this comment.
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.
Why the change here? We usually avoid mixing '_' and this.
There was a problem hiding this comment.
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.
Replaced all the fields with _ prefix and removed all usages of this.
There was a problem hiding this comment.
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 |
|
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.