Skip to content

KAFKA-20194: Add sesssion-header-store support to TDD#21956

Merged
mjsax merged 6 commits intoapache:trunkfrom
mjsax:kafka-20194-ensure-backward-compatiblity-iqV1
Apr 8, 2026
Merged

KAFKA-20194: Add sesssion-header-store support to TDD#21956
mjsax merged 6 commits intoapache:trunkfrom
mjsax:kafka-20194-ensure-backward-compatiblity-iqV1

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented Apr 3, 2026

Adds missing method TopologyTestDriver.getSessionStoreWithHeader(), and
updates TopologyTestDriverTest for all newly added header store types.

Reviewers: TengYao Chi frankvicky@apache.org, Alieh Saeedi
asaeedi@confluent.io

@mjsax mjsax added streams kip Requires or implements a KIP labels Apr 3, 2026
if (store instanceof TimestampedKeyValueStoreWithHeaders) {
if (queryableStoreType instanceof QueryableStoreTypes.KeyValueStoreType) {
return (List<T>) Collections.singletonList(new GenericReadOnlyKeyValueStoreFacade<>((TimestampedKeyValueStoreWithHeaders<Object, Object>) store, ValueConverters.extractValueFromHeaders()));
return (List<T>) Collections.singletonList(new GenericReadOnlyKeyValueStoreFacade<>((TimestampedKeyValueStoreWithHeaders<?, ?>) store, ValueConverters.extractValueFromHeaders()));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unrelated side cleanup -- same for StreamThreadStateStoreProvider below.

* @see #getTimestampedKeyValueStore(String)
* @see #getVersionedKeyValueStore(String)
* @see #getTimestampedKeyValueStoreWithHeaders(String)
* @see #getVersionedKeyValueStore(String)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just fixing the grouping of methods... Similar below.


/**
* Get the {@link VersionedKeyValueStore} with the given name.
* Get the {@link TimestampedKeyValueStoreWithHeaders} with the given name.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This diff is weird -- I moved method getTimestampedKeyValueStoreWithHeaders before getVersionedKeyValueStore to fix the grouping in the code, too.

* @see #getTimestampedKeyValueStoreWithHeaders(String)
* @see #getVersionedKeyValueStore(String)
* @see #getWindowStore(String)
* @see #getTimestampedWindowStoreWithHeaders(String)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

additional fix -- this line was missing

topology.addSource(sourceName1, Serdes.Long().deserializer(), Serdes.String().deserializer(), SOURCE_TOPIC_1);
topology.addSource(sourceName2, Serdes.Integer().deserializer(), Serdes.Double().deserializer(), SOURCE_TOPIC_2);
topology.addSource(sourceName1, new LongDeserializer(), new StringDeserializer(), SOURCE_TOPIC_1);
topology.addSource(sourceName2, new IntegerDeserializer(), new DoubleDeserializer(), SOURCE_TOPIC_2);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Side cleanup (similar elsewhere)

}
}

// CAUTION: Do not replace with Lambda; Needs to return a new Processor instance each eimte
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IntelliJ told me to refactor, and I accepted it, but it broke the test... a lambda is a singleton, which is not allowed for a Supplier. -- So Adding this comment to prevent other to make the same mistake.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool...
I think it might be optimized by JVM if we use lambda.(use the same instance)

Stores.inMemoryKeyValueStore(keyValueStoreName),
persistent
? Stores.persistentKeyValueStore(keyValueStoreName)
: Stores.inMemoryKeyValueStore(keyValueStoreName),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Formatting improvement.

),
"processor");
topology.addStateStore(
persistent ?
Copy link
Copy Markdown
Member Author

@mjsax mjsax Apr 3, 2026

Choose a reason for hiding this comment

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

Side cleanup / simplification. We can move the persistent check one level down, and re-use the same code for the outer "builder" call. (Similar below)

stateDirectory.clean();
}

static class MockChangelogRegister implements ChangelogRegister {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unused. Removing.

}
}

// CAUTION: Do not replace with Lambda; Needs to return a new Processor instance each eimte
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// CAUTION: Do not replace with Lambda; Needs to return a new Processor instance each eimte
// CAUTION: Do not replace with Lambda; Needs to return a new Processor instance each time

* @see #getSessionStore(String)
*/
@SuppressWarnings("unchecked")
public <K, V> SessionStoreWithHeaders<K, V> getSessionStoreWithHeader(final String name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public <K, V> SessionStoreWithHeaders<K, V> getSessionStoreWithHeader(final String name) {
public <K, V> SessionStoreWithHeaders<K, V> getSessionStoreWithHeaders(final String name) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If yes, we should change all the java docs as well

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, good catch. IntelliJ "rename" feature takes care of JavaDocs automatically.

Copy link
Copy Markdown
Contributor

@aliehsaeedii aliehsaeedii left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM, just some nits. Approved anyway.

* @see #getSessionStore(String)
*/
@SuppressWarnings("unchecked")
public <K, V> SessionStoreWithHeaders<K, V> getSessionStoreWithHeader(final String name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If yes, we should change all the java docs as well

Copy link
Copy Markdown
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

LGTM after addressing nits

@mjsax mjsax force-pushed the kafka-20194-ensure-backward-compatiblity-iqV1 branch from aeb3e4f to 335ecfe Compare April 8, 2026 07:38
@mjsax mjsax force-pushed the kafka-20194-ensure-backward-compatiblity-iqV1 branch from 335ecfe to 98f234f Compare April 8, 2026 16:39
@mjsax mjsax merged commit 94fc9ad into apache:trunk Apr 8, 2026
23 checks passed
@mjsax mjsax deleted the kafka-20194-ensure-backward-compatiblity-iqV1 branch April 8, 2026 18:47
mjsax added a commit that referenced this pull request Apr 8, 2026
Adds missing method TopologyTestDriver.getSessionStoreWithHeader(), and
updates  TopologyTestDriverTest for all newly added header store types.

Reviewers: TengYao Chi <frankvicky@apache.org>, Alieh Saeedi
 <asaeedi@confluent.io>
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Apr 8, 2026

Merged to trunk and cherry-picked to 4.3 branch.

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

Development

Successfully merging this pull request may close these issues.

3 participants