Skip to content

Simplify code and error handling between ActivityWorker and LocalActivityWorker #1263

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

Spikhalskiy
Copy link
Contributor

Simplify code and error handling between ActivityWorker and LocalActivityWorker
Make LocalActivityWorker more robust to unexpected errors

Partially addresses #1262

@Spikhalskiy Spikhalskiy force-pushed the activity-worker-refactoring branch 4 times, most recently from 9a896ee to 815c464 Compare June 16, 2022 18:56
@Spikhalskiy Spikhalskiy marked this pull request as ready for review June 16, 2022 18:56
@Spikhalskiy Spikhalskiy force-pushed the activity-worker-refactoring branch from 815c464 to e5582db Compare June 16, 2022 18:57
Comment on lines +185 to +187
// TODO should go away with SimulatedTimeoutFailure
if (exception instanceof TimeoutFailure) {
exception = new SimulatedTimeoutFailure((TimeoutFailure) exception);
Copy link
Member

Choose a reason for hiding this comment

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

Todo refers to thing that it implies doesn't exist, but seems to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will go away in the next PR, didn't want to throw everything into one.

Comment on lines 289 to 290
// For small backoff we do local retry. Otherwise we will schedule timer on server side.
// TODO(maxim): Use timer queue for retries to avoid tying up a thread.
Copy link
Member

Choose a reason for hiding this comment

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

Did you find where this implementation actually is, if anywhere?

Copy link
Contributor Author

@Spikhalskiy Spikhalskiy Jun 17, 2022

Choose a reason for hiding this comment

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

It was actually in the code base but was removed during fat finger refactoring more than 2 years ago. #1261
Will need to bring it back in a backward-compatible manner.

@Spikhalskiy Spikhalskiy force-pushed the activity-worker-refactoring branch 2 times, most recently from 3483b7c to f026ba1 Compare June 17, 2022 05:17
@cretz
Copy link
Member

cretz commented Jun 17, 2022

To save me a bit of time, can you clarify which parts are code moves and which parts are new code I should focus on?

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Jun 17, 2022

@cretz It's all code restructuring, deduplication by introducing parent classes, unification of implementation between workers, and ensuring that callback is always called for LAs. There is pretty much no new code.

@Spikhalskiy Spikhalskiy force-pushed the activity-worker-refactoring branch from f026ba1 to f28cbc6 Compare June 17, 2022 14:05
…vityWorker

Make LocalActivityWorker more robust to unexpected errors
Issue temporalio#1262
@Spikhalskiy Spikhalskiy force-pushed the activity-worker-refactoring branch from f28cbc6 to 0fd824f Compare June 17, 2022 16:11
@Spikhalskiy Spikhalskiy merged commit 8f27901 into temporalio:master Jun 17, 2022
@Spikhalskiy Spikhalskiy deleted the activity-worker-refactoring branch June 17, 2022 16:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants