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

MINOR: deleteHorizonMs update to documentation and DumpLogSegments tool #11694

Merged
merged 3 commits into from
Feb 5, 2022

Conversation

mattwong949
Copy link
Contributor

@mattwong949 mattwong949 commented Jan 20, 2022

This PR updates the documentation and tooling to match the changes made in #10914

In the documentation, changes include the adding the new attribute and updating field names.

In the DumpLogSegments tool, when record batch information is printed, it will also include what the value of deleteHorizonMs is ( OptionalLong.empty or OptionalLong[123456] )

Copy link
Contributor

@vincent81jiang vincent81jiang left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

Copy link

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks. While we're updating documentation, there are some comments about the old FirstTimestamp field in DefaultRecordBatch that seem outdated.

* field therefore always reflects the timestamp of the first record in the batch. If the batch is empty, the
* FirstTimestamp will be set to -1 (NO_TIMESTAMP).
* BaseTimestamp will be set to -1 (NO_TIMESTAMP).

Choose a reason for hiding this comment

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

The comment is still inaccurate, no? The base timestamp does not always reflect the timestamp of the first record in the batch.

Copy link
Contributor Author

@mattwong949 mattwong949 Jan 25, 2022

Choose a reason for hiding this comment

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

you are right, thanks for the catch. I've revised the comments to be accurate about BaseTimestamp after compaction.

Copy link
Contributor

@Kvicii Kvicii left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@hachikuji hachikuji 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 for the patch!

@hachikuji hachikuji merged commit 17dcb80 into apache:trunk Feb 5, 2022
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.

4 participants