Temporal POC #3778

y242yang · 2023-09-19T01:29:56Z

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

This PR adds temporal support and runs temporal workers on top of yarn containers. Because it does not affect any existing behaviors, there is no gobblin JIRA addressing this.

Description

We start temporal workers and assign 1 worker per yarn container.

Tests

E2E testing is done with customized temporal workflow and temporal workers.

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

phet

nice work, ann... but very large review here.
hence this review is only part one: I'll pause now and return later to finish

phet · 2023-09-19T06:06:20Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterConfigurationKeys.java

@@ -234,5 +234,7 @@ public class GobblinClusterConfigurationKeys {

 public static final String HELIX_JOB_SCHEDULING_THROTTLE_TIMEOUT_SECONDS_KEY = "helix.job.scheduling.throttle.timeout.seconds";
 public static final long DEFAULT_HELIX_JOB_SCHEDULING_THROTTLE_TIMEOUT_SECONDS_KEY = Duration.ofMinutes(40).getSeconds();;
-
+ public static final String TEMPORAL_WORKER_SIZE = "temporal.worker.size";


let's choose an unambiguous name. is this num workers or size of each one? if the latter, what units are we discussing (e.g. mb of memory)?

phet · 2023-09-19T06:09:58Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java

+ * This class runs the {@link GobblinHelixJobScheduler} for scheduling
+ * and running Gobblin jobs.


if we're not changing anything here, leave this file off the commit.

non-substantive, formatting-only changes are OK, but belong in a separate commit

phet · 2023-09-19T06:18:39Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobLauncherMetrics.java

@@ -28,7 +28,7 @@
 /**
 * Metrics that relates to jobs launched by {@link GobblinHelixJobLauncher}.
 */
-class GobblinHelixJobLauncherMetrics extends StandardMetricsBridge.StandardMetrics {
+public class GobblinHelixJobLauncherMetrics extends StandardMetricsBridge.StandardMetrics {


I understand the need to call from the temporal sub-package you've created... but that sidesteps the potential confusion of using "helix-named" classes, when we're not actually involved w/ helix at runtime.

a good compromise for this PR, might be to add a TODO to all three--this, the job scheduler metrics, and the GobblinHelixJobLauncherListener--about renaming to be helix/temporal agnostic... (or if merited to instead create a separate temporal-specific variant)

phet · 2023-09-19T06:26:10Z

...uster/src/main/java/org/apache/gobblin/cluster/temporal/AbstractNestingExecWorkflowImpl.java

this and a bunch of other files represent cumulatively a specific worker w/ its particular workflows and activities (plus supporting abstractions, like Workload and WFAddr)--let's put them into their own package, separate from the worker-agnostic scaffolding for running arbitrary workers.

I know we presently hard-code this one, but our next follow-on PR will be to load whatever configured specific worker using reflection. separating today's "example worker" from the reusable scaffolding, anticipates that future.

phet · 2023-09-19T06:31:35Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/temporal/NestingExecWorkflowImpl.java

+public class NestingExecWorkflowImpl
+ extends AbstractNestingExecWorkflowImpl<IllustrationTask, String> {


naming-wise, this is really just one among a family of NestingExecWorkflow impls, the one for IllustrationTasks.

the class name should reflect that

phet · 2023-09-19T07:16:28Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinJobLauncher.java

+import org.apache.gobblin.util.SerializationUtils;
+
+/**
+ * An implementation of {@link JobLauncher} that launches a Gobblin job using the Temporal task framework.


it sounds here like temporal is the fundamental proposition, but I don't immediately see where it comes in within this class def. is the class actually more general? please update the javadoc to guide on where that stands

phet · 2023-09-19T07:24:20Z

...in-cluster/src/main/java/org/apache/gobblin/cluster/temporal/GobblinTemporalJobLauncher.java

+@Slf4j
+public class GobblinTemporalJobLauncher extends GobblinJobLauncher {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(GobblinTemporalJobLauncher.class);


again, isn't this redundant, given @Slf4j?

phet · 2023-09-19T07:32:39Z

...n-cluster/src/main/java/org/apache/gobblin/cluster/temporal/GobblinTemporalJobScheduler.java

+ final GobblinHelixJobSchedulerMetrics jobSchedulerMetrics;
+ final GobblinHelixJobLauncherMetrics launcherMetrics;
+ final GobblinHelixPlanningJobLauncherMetrics planningJobLauncherMetrics;
+ final HelixJobsMapping jobsMapping;


noting again, what I previously mentioned: there's a confusing amount of reference to helix, given the framework isn't actually involved in execution

phet · 2023-09-19T07:33:32Z

...n-cluster/src/main/java/org/apache/gobblin/cluster/temporal/GobblinTemporalJobScheduler.java

+ public GobblinTemporalJobScheduler(Config sysConfig,
+ EventBus eventBus,
+ Path appWorkDir, List<? extends Tag<?>> metadataTags,
+ SchedulerService schedulerService) throws Exception {


line-continuation indentation should be four spaces. see the GobblinHelixJobLauncher ctor as an example

phet · 2023-09-19T07:40:27Z

...arn/src/main/java/org/apache/gobblin/yarn/temporal/YarnTemporalAppMasterSecurityManager.java

+import org.apache.gobblin.yarn.event.DelegationTokenUpdatedEvent;
+
+
+public class YarnTemporalAppMasterSecurityManager extends YarnContainerSecurityManager{


needs javadoc

y242yang added 4 commits September 18, 2023 10:37

temporal poc

9b87239

add temporal classes

5531b4d

remove dependency

c71d4da

point to the right application master

a17378d

phet reviewed Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporal POC #3778

Temporal POC #3778

y242yang commented Sep 19, 2023

phet left a comment •

edited

phet Sep 19, 2023

phet Sep 19, 2023

phet Sep 19, 2023

phet Sep 19, 2023 •

edited

phet Sep 19, 2023 •

edited

phet Sep 19, 2023

phet Sep 19, 2023 •

edited

phet Sep 19, 2023

phet Sep 19, 2023

phet Sep 19, 2023

		* This class runs the {@link GobblinHelixJobScheduler} for scheduling
		* and running Gobblin jobs.

		public class NestingExecWorkflowImpl
		extends AbstractNestingExecWorkflowImpl<IllustrationTask, String> {

		import org.apache.gobblin.yarn.event.DelegationTokenUpdatedEvent;


		public class YarnTemporalAppMasterSecurityManager extends YarnContainerSecurityManager{

Temporal POC #3778

Are you sure you want to change the base?

Temporal POC #3778

Conversation

y242yang commented Sep 19, 2023

JIRA

Description

Tests

Commits

phet left a comment • edited

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023 • edited

Choose a reason for hiding this comment

phet Sep 19, 2023 • edited

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023 • edited

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet Sep 19, 2023

Choose a reason for hiding this comment

phet left a comment •

edited

phet Sep 19, 2023 •

edited

phet Sep 19, 2023 •

edited

phet Sep 19, 2023 •

edited