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: [10/N] Add changelogging wrapper for versioned stores #13251

Merged
merged 2 commits into from Feb 21, 2023

Conversation

vcrfxia
Copy link
Collaborator

@vcrfxia vcrfxia commented Feb 15, 2023

This PR introduces the changelogging layer for the new versioned key-value store introduced in KIP-889. We choose to have the changelogging layer operate on VersionedBytesStore rather than VersionedKeyValueStore so that the outermost store layer (metered store, see #13252 for more) can serialize to bytes once and then all inner stores operate only with bytes types.

Changelog topic configs for versioned stores need to be slightly different than for non-versioned key-value stores. This change will be made in a follow-up PR.

Committer Checklist (excluded from commit message)

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

Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM. Two questions.

ChangeLoggingVersionedKeyValueBytesStore(final KeyValueStore<Bytes, byte[]> inner) {
super(inner);
if (!(inner instanceof VersionedBytesStore)) {
throw new IllegalStateException("inner store must be versioned");
Copy link
Member

Choose a reason for hiding this comment

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

IllegalArgumentException ?

public void shouldPropagateAndLogOnPutNull() {
final Bytes rawKey = Bytes.wrap(rawBytes("k"));
final long timestamp = 10L;
final byte[] rawValueAndTimestamp = rawValueAndTimestamp(null, timestamp);
Copy link
Member

Choose a reason for hiding this comment

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

rawTombstoneAndTimestamp ?

@mjsax mjsax added streams kip Requires or implements a KIP labels Feb 17, 2023
@mjsax mjsax merged commit a2c9f42 into apache:trunk Feb 21, 2023
@vcrfxia vcrfxia deleted the kip-889-changelogging-store branch February 22, 2023 06:05
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