Skip to content
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

Temporal Worker Wrapper #2368

Merged
merged 3 commits into from
Mar 9, 2021
Merged

Temporal Worker Wrapper #2368

merged 3 commits into from
Mar 9, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Mar 9, 2021

What

  • Wrap execution of Workers when executed in Temporal. Specifically move all the logic for 1. job root / log paths 2. MDCs 3. error handling in one place, so it can all be tested once.

How

  • Wrap execution of Workers in TemporalAttemptExecution.
  • The other thing that's happening in here is I'm moving all the temporal stuff into the workers. I shouldn't have put that in this PR but it got mixed in and it's more hassle than it's worth to separate it now.
  • Also the serializable pojos that are specific to temporal are now moved into the workers module (from config)

Pre-merge Checklist

  • actually write the tests

Recommended reading order

  1. TemporalAttemptExecution.java
  2. the rest

@@ -52,33 +60,49 @@ public WorkerRun create(Job job) {
return WorkerRun.create(workspaceRoot, job.getId(), attemptId, createSupplier(job, attemptId));
}

@SuppressWarnings("UnnecessaryDefault")
@SuppressWarnings({"UnnecessaryDefault", "CodeBlock2Expr"})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the formatting makes this really hard to read if you don't have the return statements (even though the bracket-less lambda syntax is technically possible in this case).

* outputs are passed to the selected worker. It also makes sures that the outputs of the worker are
* persisted to the db.
*/
public class TemporalAttemptExecution<T> implements CheckedSupplier<T, TemporalJobException> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to with a base temporal activity that could be extend to maximize how much code we could de-duplicate, but i couldn't find a version of it that i liked. it is particularly tricky given the interfaces that you need to work around with temporal. instead I did the TemporalAttemptExecution wrapper.

@cgardens cgardens requested a review from jrhizor March 9, 2021 02:55
@cgardens cgardens marked this pull request as ready for review March 9, 2021 02:55
Base automatically changed from cgardens/tng_worker to master March 9, 2021 21:07
@cgardens cgardens merged commit b7abc5b into master Mar 9, 2021
@cgardens cgardens deleted the cgardens/tng_worker10 branch March 9, 2021 21:09
@cgardens cgardens mentioned this pull request Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants