-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
package org.apache.gobblin.cluster; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
import org.apache.hadoop.fs.FileSystem; | ||
import org.apache.hadoop.fs.Path; | ||
|
@@ -31,26 +31,16 @@ | |
import org.slf4j.MDC; | ||
|
||
import com.google.common.base.Throwables; | ||
import com.google.common.collect.Lists; | ||
import com.google.common.io.Closer; | ||
import com.typesafe.config.ConfigFactory; | ||
|
||
import org.apache.gobblin.annotation.Alpha; | ||
import org.apache.gobblin.broker.SharedResourcesBrokerFactory; | ||
import org.apache.gobblin.broker.gobblin_scopes.GobblinScopeTypes; | ||
import org.apache.gobblin.broker.gobblin_scopes.JobScopeInstance; | ||
import org.apache.gobblin.broker.iface.SharedResourcesBroker; | ||
import org.apache.gobblin.configuration.ConfigurationKeys; | ||
import org.apache.gobblin.runtime.AbstractJobLauncher; | ||
import org.apache.gobblin.runtime.GobblinMultiTaskAttempt; | ||
import org.apache.gobblin.runtime.JobState; | ||
import org.apache.gobblin.runtime.TaskState; | ||
import org.apache.gobblin.runtime.util.StateStores; | ||
import org.apache.gobblin.source.workunit.MultiWorkUnit; | ||
import org.apache.gobblin.source.workunit.WorkUnit; | ||
import org.apache.gobblin.util.Id; | ||
import org.apache.gobblin.util.JobLauncherUtils; | ||
import org.apache.gobblin.util.SerializationUtils; | ||
|
||
|
||
/** | ||
|
@@ -67,113 +57,63 @@ | |
* {@link org.apache.gobblin.runtime.Task}(s), it persists the {@link TaskState} of each {@link org.apache.gobblin.runtime.Task} to | ||
* a file that will be collected by the {@link GobblinHelixJobLauncher} later upon completion of the job. | ||
* </p> | ||
* | ||
* @author Yinan Li | ||
*/ | ||
@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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. See comment above. |
||
|
||
private final TaskConfig taskConfig; | ||
// An empty JobState instance that will be filled with values read from the serialized JobState | ||
private final JobState jobState = new JobState(); | ||
private final String jobName; | ||
private final String jobId; | ||
private final String jobKey; | ||
|
||
private final FileSystem fs; | ||
private final StateStores stateStores; | ||
private final TaskAttemptBuilder taskAttemptBuilder; | ||
private String jobName; | ||
private String jobId; | ||
private String jobKey; | ||
private Path workUnitFilePath; | ||
|
||
private GobblinMultiTaskAttempt taskAttempt; | ||
private SingleHelixTask task; | ||
|
||
public GobblinHelixTask(TaskCallbackContext taskCallbackContext, FileSystem fs, Path appWorkDir, | ||
TaskAttemptBuilder taskAttemptBuilder, StateStores stateStores) | ||
throws IOException { | ||
|
||
this.taskConfig = taskCallbackContext.getTaskConfig(); | ||
this.stateStores = stateStores; | ||
this.taskAttemptBuilder = taskAttemptBuilder; | ||
this.jobName = this.taskConfig.getConfigMap().get(ConfigurationKeys.JOB_NAME_KEY); | ||
this.jobId = this.taskConfig.getConfigMap().get(ConfigurationKeys.JOB_ID_KEY); | ||
this.jobKey = Long.toString(Id.parse(this.jobId).getSequence()); | ||
getInfoFromTaskConfig(); | ||
|
||
Path jobStateFilePath = constructJobStateFilePath(appWorkDir); | ||
|
||
this.fs = fs; | ||
this.task = | ||
new SingleHelixTask(this.jobId, workUnitFilePath, jobStateFilePath, fs, taskAttemptBuilder, stateStores); | ||
} | ||
|
||
Path jobStateFilePath = new Path(appWorkDir, this.jobId + "." + AbstractJobLauncher.JOB_STATE_FILE_NAME); | ||
SerializationUtils.deserializeState(this.fs, jobStateFilePath, this.jobState); | ||
private Path constructJobStateFilePath(Path appWorkDir) { | ||
return new Path(appWorkDir, this.jobId + "." + AbstractJobLauncher.JOB_STATE_FILE_NAME); | ||
} | ||
|
||
private void getInfoFromTaskConfig() { | ||
Map<String, String> configMap = this.taskConfig.getConfigMap(); | ||
this.jobName = configMap.get(ConfigurationKeys.JOB_NAME_KEY); | ||
this.jobId = configMap.get(ConfigurationKeys.JOB_ID_KEY); | ||
this.jobKey = Long.toString(Id.parse(this.jobId).getSequence()); | ||
this.workUnitFilePath = new Path(configMap.get(GobblinClusterConfigurationKeys.WORK_UNIT_FILE_PATH)); | ||
} | ||
|
||
@Override | ||
public TaskResult run() { | ||
SharedResourcesBroker<GobblinScopeTypes> globalBroker = null; | ||
try (Closer closer = Closer.create()) { | ||
closer.register(MDC.putCloseable(ConfigurationKeys.JOB_NAME_KEY, this.jobName)); | ||
closer.register(MDC.putCloseable(ConfigurationKeys.JOB_KEY_KEY, this.jobKey)); | ||
Path workUnitFilePath = | ||
new Path(this.taskConfig.getConfigMap().get(GobblinClusterConfigurationKeys.WORK_UNIT_FILE_PATH)); | ||
|
||
String fileName = workUnitFilePath.getName(); | ||
String storeName = workUnitFilePath.getParent().getName(); | ||
WorkUnit workUnit; | ||
|
||
if (workUnitFilePath.getName().endsWith(AbstractJobLauncher.MULTI_WORK_UNIT_FILE_EXTENSION)) { | ||
workUnit = stateStores.mwuStateStore.getAll(storeName, fileName).get(0); | ||
} else { | ||
workUnit = stateStores.wuStateStore.getAll(storeName, fileName).get(0); | ||
} | ||
|
||
// The list of individual WorkUnits (flattened) to run | ||
List<WorkUnit> workUnits = Lists.newArrayList(); | ||
|
||
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); | ||
} else { | ||
workUnits.add(workUnit); | ||
} | ||
|
||
globalBroker = SharedResourcesBrokerFactory.createDefaultTopLevelBroker( | ||
ConfigFactory.parseProperties(this.jobState.getProperties()), GobblinScopeTypes.GLOBAL.defaultScopeInstance()); | ||
SharedResourcesBroker<GobblinScopeTypes> jobBroker = | ||
globalBroker.newSubscopedBuilder(new JobScopeInstance(this.jobState.getJobName(), this.jobState.getJobId())).build(); | ||
|
||
this.taskAttempt = this.taskAttemptBuilder.build(workUnits.iterator(), this.jobId, this.jobState, jobBroker); | ||
this.taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE); | ||
return new TaskResult(TaskResult.Status.COMPLETED, String.format("completed tasks: %d", workUnits.size())); | ||
int workUnitSize = this.task.run(); | ||
return new TaskResult(TaskResult.Status.COMPLETED, String.format("completed tasks: %d", workUnitSize)); | ||
} catch (InterruptedException ie) { | ||
Thread.currentThread().interrupt(); | ||
return new TaskResult(TaskResult.Status.CANCELED, ""); | ||
} catch (Throwable t) { | ||
LOGGER.error("GobblinHelixTask failed due to " + t.getMessage(), t); | ||
return new TaskResult(TaskResult.Status.ERROR, Throwables.getStackTraceAsString(t)); | ||
} finally { | ||
if (globalBroker != null) { | ||
try { | ||
globalBroker.close(); | ||
} catch (IOException ioe) { | ||
LOGGER.error("Could not close shared resources broker.", ioe); | ||
} | ||
} | ||
_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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. ERROR has been deprecated and replaced with FAILED. |
||
} | ||
} | ||
|
||
@Override | ||
public void cancel() { | ||
if (this.taskAttempt != null) { | ||
try { | ||
LOGGER.info("Task cancelled: Shutdown starting for tasks with jobId: {}", this.jobId); | ||
this.taskAttempt.shutdownTasks(); | ||
LOGGER.info("Task cancelled: Shutdown complete for tasks with jobId: {}", this.jobId); | ||
} catch (InterruptedException e) { | ||
throw new RuntimeException("Interrupted while shutting down task with jobId: " + this.jobId, e); | ||
} | ||
} else { | ||
LOGGER.error("Task cancelled but taskAttempt is null, so ignoring."); | ||
} | ||
this.task.cancel(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.gobblin.cluster; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
import java.util.Properties; | ||
|
||
import org.apache.hadoop.fs.FileSystem; | ||
import org.apache.hadoop.fs.Path; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import com.google.common.collect.Lists; | ||
import com.typesafe.config.Config; | ||
import com.typesafe.config.ConfigFactory; | ||
|
||
import org.apache.gobblin.broker.SharedResourcesBrokerFactory; | ||
import org.apache.gobblin.broker.gobblin_scopes.GobblinScopeTypes; | ||
import org.apache.gobblin.broker.gobblin_scopes.JobScopeInstance; | ||
import org.apache.gobblin.broker.iface.SharedResourcesBroker; | ||
import org.apache.gobblin.runtime.AbstractJobLauncher; | ||
import org.apache.gobblin.runtime.GobblinMultiTaskAttempt; | ||
import org.apache.gobblin.runtime.JobState; | ||
import org.apache.gobblin.runtime.util.StateStores; | ||
import org.apache.gobblin.source.workunit.MultiWorkUnit; | ||
import org.apache.gobblin.source.workunit.WorkUnit; | ||
import org.apache.gobblin.util.JobLauncherUtils; | ||
import org.apache.gobblin.util.SerializationUtils; | ||
|
||
|
||
public class SingleHelixTask { | ||
|
||
private static final Logger _logger = LoggerFactory.getLogger(SingleHelixTask.class); | ||
|
||
private GobblinMultiTaskAttempt _taskattempt; | ||
private String _jobId; | ||
private Path _workUnitFilePath; | ||
private Path _jobStateFilePath; | ||
private FileSystem _fs; | ||
private TaskAttemptBuilder _taskAttemptBuilder; | ||
private StateStores _stateStores; | ||
|
||
SingleHelixTask(String jobId, Path workUnitFilePath, Path jobStateFilePath, FileSystem fs, | ||
TaskAttemptBuilder taskAttemptBuilder, StateStores stateStores) { | ||
_jobId = jobId; | ||
_workUnitFilePath = workUnitFilePath; | ||
_jobStateFilePath = jobStateFilePath; | ||
_fs = fs; | ||
_taskAttemptBuilder = taskAttemptBuilder; | ||
_stateStores = stateStores; | ||
} | ||
|
||
/** | ||
* | ||
* @return the number of work-units processed | ||
* @throws IOException | ||
* @throws InterruptedException | ||
*/ | ||
public int run() | ||
throws IOException, InterruptedException { | ||
List<WorkUnit> workUnits = getWorkUnits(); | ||
int workUnitSize = workUnits.size(); | ||
|
||
JobState jobState = getJobState(); | ||
Config jobConfig = getConfigFromJobState(jobState); | ||
|
||
try (SharedResourcesBroker<GobblinScopeTypes> globalBroker = SharedResourcesBrokerFactory | ||
.createDefaultTopLevelBroker(jobConfig, GobblinScopeTypes.GLOBAL.defaultScopeInstance())) { | ||
SharedResourcesBroker<GobblinScopeTypes> jobBroker = getJobBroker(jobState, globalBroker); | ||
|
||
_taskattempt = _taskAttemptBuilder.build(workUnits.iterator(), _jobId, jobState, jobBroker); | ||
_taskattempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE); | ||
return workUnitSize; | ||
} | ||
} | ||
|
||
private SharedResourcesBroker<GobblinScopeTypes> getJobBroker(JobState jobState, | ||
SharedResourcesBroker<GobblinScopeTypes> globalBroker) { | ||
return globalBroker.newSubscopedBuilder(new JobScopeInstance(jobState.getJobName(), jobState.getJobId())).build(); | ||
} | ||
|
||
private Config getConfigFromJobState(JobState jobState) { | ||
Properties jobProperties = jobState.getProperties(); | ||
return ConfigFactory.parseProperties(jobProperties); | ||
} | ||
|
||
private JobState getJobState() | ||
throws java.io.IOException { | ||
JobState jobState = new JobState(); | ||
SerializationUtils.deserializeState(_fs, _jobStateFilePath, jobState); | ||
return jobState; | ||
} | ||
|
||
private List<WorkUnit> getWorkUnits() | ||
throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. IDE auto formatted it based on the style config Gobblin provides. |
||
String fileName = _workUnitFilePath.getName(); | ||
String storeName = _workUnitFilePath.getParent().getName(); | ||
WorkUnit workUnit; | ||
|
||
if (_workUnitFilePath.getName().endsWith(AbstractJobLauncher.MULTI_WORK_UNIT_FILE_EXTENSION)) { | ||
workUnit = _stateStores.mwuStateStore.getAll(storeName, fileName).get(0); | ||
} else { | ||
workUnit = _stateStores.wuStateStore.getAll(storeName, fileName).get(0); | ||
} | ||
|
||
// The list of individual WorkUnits (flattened) to run | ||
List<WorkUnit> workUnits = Lists.newArrayList(); | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, |
||
} else { | ||
workUnits.add(workUnit); | ||
} | ||
return workUnits; | ||
} | ||
|
||
public void cancel() { | ||
if (_taskattempt != null) { | ||
try { | ||
_logger.info("Task cancelled: Shutdown starting for tasks with jobId: {}", _jobId); | ||
_taskattempt.shutdownTasks(); | ||
_logger.info("Task cancelled: Shutdown complete for tasks with jobId: {}", _jobId); | ||
} catch (InterruptedException e) { | ||
throw new RuntimeException("Interrupted while shutting down task with jobId: " + _jobId, e); | ||
} | ||
} else { | ||
_logger.error("Task cancelled but _taskattempt is null, so ignoring."); | ||
} | ||
} | ||
} |
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?