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

KAFKA-14491: [4/N] Improvements to segment value format for RocksDB versioned store #13186

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

vcrfxia
Copy link
Collaborator

@vcrfxia vcrfxia commented Feb 2, 2023

Minor changes to the segment value format introduced in #13126, as a follow-up to the latest PR review.

  • "segment" -> "segment row" in javadocs, where appropriate
  • additional exposition regarding the "degenerate segment row" edge case
  • removal of all non-top-level javadoc mentions of the degenerate row case
  • other minor stylistic changes and additional clarifications

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

* the relevant index (to insert into index n requires that index n-1 has already been deserialized).
* Inserts the provided record into the segment row at the provided index. This operation
* requires that {@link SegmentValue#find(long, boolean)} has already been called in order to deserialize
* the relevant index (to insert into index n requires that index n has already been deserialized).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The requirement change from n-1 to n is because using n-1 means callers could attempt to call this method on a degenerate segment, which would not be valid. In practice, callers never have reason to call this method with n-1; only the internal implementation of insertAsLatest() has reason to do this, but this method calls the helper method doInsert() directly instead.

By updating this requirement from n-1 to n, it ensures that callers will never be allowed to call this method on a degenerate segment, and that note can be removed from the javadocs as a result.

@mjsax mjsax added streams kip Requires or implements a KIP labels Feb 3, 2023
@mjsax mjsax merged commit b8e6063 into apache:trunk Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kip Requires or implements a KIP streams
Projects
None yet
2 participants