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.

Sign up for GitHub

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

Jump to bottom

Make max.pending.records property reloadable #207

Merged
merged 4 commits into from
Jul 20, 2023
Merged

Make max.pending.records property reloadable #207

merged 4 commits into from
Jul 20, 2023

Conversation

KarboniteKream
Copy link
Contributor

This PR updates PartitionContexts so the max.pending.records property can be reloaded at runtime, similar to the partition.concurrency property.

Modifications:

  • Extract reload listener in PartitionContexts and register it for CONFIG_MAX_PENDING_RECORDS
  • PartitionContext now reads the property value from PartitionScope
  • Cleanup ProcessorProperties Javadoc formatting
  • Cleanup some unchecked code in tests

@CLAassistant
Copy link

CLAassistant commented Jun 24, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@KarboniteKream KarboniteKream force-pushed the feat/max-pending-records-reload branch from 9cff79b to a8f71f2 Compare June 24, 2023 15:25
@KarboniteKream KarboniteKream force-pushed the feat/max-pending-records-reload branch from a8f71f2 to 90c51e8 Compare June 24, 2023 15:40
@ocadaruma ocadaruma self-requested a review July 11, 2023 04:56
ocadaruma
ocadaruma reviewed Jul 13, 2023
View reviewed changes
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.

Sorry for delaying the review.

Thanks! Left a comment but overall looks good.
Also could you add an integration test in PropertyReloadRequestTest to check dynamic max.pending.records reload works?

processor/src/main/java/com/linecorp/decaton/processor/runtime/internal/PartitionContexts.java Outdated Show resolved Hide resolved
ocadaruma
ocadaruma reviewed Jul 19, 2023
View reviewed changes
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! Left only nits comment

processor/src/it/java/com/linecorp/decaton/processor/PropertyReloadRequestTest.java Outdated Show resolved Hide resolved
ocadaruma
ocadaruma approved these changes Jul 20, 2023
View reviewed changes
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, thanks!

@ocadaruma ocadaruma merged commit 44108c5 into line:master Jul 20, 2023
@KarboniteKream KarboniteKream deleted the feat/max-pending-records-reload branch July 20, 2023 05:31
@ocadaruma ocadaruma added the breaking change Breaking change for a public API label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocadaruma ocadaruma ocadaruma approved these changes

Assignees
No one assigned
Labels
breaking change Breaking change for a public API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KarboniteKream @CLAassistant @ocadaruma