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

ProcessorSubscription refactoring #60

Merged
merged 6 commits into from
Dec 24, 2020

Conversation

kawamuray
Copy link
Contributor

@kawamuray kawamuray commented Jul 31, 2020

As we talked in #53 , ProcessorSubscription's getting bigger and complex by many functions being added to it.
I've attempted to refactor it by mainly extracting things into a separate classes.
The work is still in progress and I'm still considering what would be the best design (especially for TaskConsumer).
Please give early feedback if any!

Some notable changes that might impacts decaton's behavior:

  • Stopped checking "offset regression" in rebalance listener and replaced it with OffsetRegressionException + on-demand repair.
  • contexts.maybeHandlePropertyReload() now happens after updateHighWatermark, pause, resume

@kawamuray kawamuray requested a review from ocadaruma July 31, 2020 11:19
@kawamuray kawamuray force-pushed the subsc-refactoring branch from 9fef5d5 to 9573c31 Compare July 31, 2020 11:19
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Thanks!

Overall design seems very good to me.
It seems ProcessorSubscription has become much testable.

@kawamuray
Copy link
Contributor Author

@ocadaruma sorry for orphaning this for a long time. I've revised patch onto latest master and made some improvements. PTAL when you have time.

@kawamuray kawamuray requested a review from ocadaruma December 22, 2020 11:04
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Almost LGTM.
Just a single minor comment.

@Value
@Accessors(fluent = true)
static
class AssignmentConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nits] We don't break line between static class I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm.. how could this happen. will fix, thx.

@kawamuray kawamuray requested a review from ocadaruma December 24, 2020 07:05
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!!

@ocadaruma ocadaruma merged commit 6d2e062 into line:master Dec 24, 2020
@kawamuray kawamuray mentioned this pull request Jan 27, 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