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

[improve] [broker] Enable AppendBrokerTimestampMetadataInterceptor by default #21939

Closed
wants to merge 10 commits into from

Conversation

315157973
Copy link
Contributor

@315157973 315157973 commented Jan 22, 2024

Main Issue: #21347

Motivation

When the client machine's clock is incorrect (eg: set to 1 year later) and the Broker does not set the AppendBrokerTimestampMetadataInterceptor, the Ledger will not be cleaned up.
Because if the Broker's timestamp is not set, the expiration check will be based on the client's publish time.

As we discuss in https://lists.apache.org/thread/0vtjt436xvlg0fr4ogchkjdvjcp56mvz

Modifications

Enable AppendBrokerTimestampMetadataInterceptor by default

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

  • The default values of configurations

  • doc-not-needed

Matching PR in forked repository

PR in forked repository: 315157973#13

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jan 22, 2024
@315157973 315157973 self-assigned this Jan 22, 2024
@315157973 315157973 added doc-not-needed Your PR changes do not impact docs and removed doc-required Your PR changes impact docs and you will update later. labels Jan 22, 2024
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Jan 22, 2024
Copy link

@315157973 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jan 22, 2024
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM. But do we need a PIP to change the default config value?

Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

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

Since the issue already fixed by #21940, do we still need this PR?

@315157973 315157973 closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants