Skip to content

KAFKA-20304: Implement all required ReadOnly*Facade classes for headers stores#21743

Open
aliehsaeedii wants to merge 6 commits intoapache:trunkfrom
aliehsaeedii:headers_stors_dsl
Open

KAFKA-20304: Implement all required ReadOnly*Facade classes for headers stores#21743
aliehsaeedii wants to merge 6 commits intoapache:trunkfrom
aliehsaeedii:headers_stors_dsl

Conversation

@aliehsaeedii
Copy link
Contributor

@aliehsaeedii aliehsaeedii commented Mar 13, 2026

This PR

  • adds all ReadOnly*Facade classes required to wrap headers store for
    backward compatibility
  • refactors ReadOnly*Facade classes to use generic infrastructure with
    converter functions

Reviewers: Matthias J. Sax matthias@confluent.io

@github-actions github-actions bot added streams triage PRs from the community labels Mar 13, 2026
Copy link
Contributor Author

@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.

I'm wondering if it makes sense to remove

  • ReadOnlyKeyValueStoreFacadeTest (exists)
  • ReadOnlyWindowStoreFacadeTest (exists)
  • KeyValueIteratorFacadeTest (exists)

just to be consistent.

@mjsax mjsax added kip Requires or implements a KIP ci-approved labels Mar 14, 2026
@mjsax
Copy link
Member

mjsax commented Mar 14, 2026

Yes, I had the same thought about removing these tests even before I read yoru comment. We can do a follow up PR or add here. Whatever you prefer.

As a matter of fact, I am even wondering if we need to add all these sub-classes of the GenericXxx classes? How often do we use these sub-classes As you test code shows, we can easily instantiate GenericXxx directly, so I am wondering if we gain much by having all the sub-classes or if we could drop them, and let each user just use GenericXxx directly?

@aliehsaeedii
Copy link
Contributor Author

Yes, I had the same thought about removing tests even before I read you comment. We can do a follow up PR or add here. Whatever you prefer.

As a matter of fact, I am even wondering if we need to add all these sub-classes of the GenericXxx classes? How often do we use these sub-classes As you test code shows, we can easily instantiate GenericXxx directly, so I am wondering if we gain much by having all the sub-classes or if we could drop them, and let each user just use GenericXxx directly?

The only real benefit of the subclasses is for TopologyTestDriver inheritance:

  • TopologyTestDriver.KeyValueStoreFacade extends ReadOnlyKeyValueStoreFacade
  • It needs the protected TimestampedKeyValueStore<K, V> inner field to call write methods
    But you are right, we could refactor this pattern to use composition instead of inheritance

static class KeyValueStoreFacade<K, V> extends ReadOnlyKeyValueStoreFacade<K, V> implements KeyValueStore<K, V> {
static class KeyValueStoreFacade<K, V> implements KeyValueStore<K, V> {
private final TimestampedKeyValueStore<K, V> inner;
private final GenericReadOnlyKeyValueStoreFacade<K, ValueAndTimestamp<V>, V> readFacade;
Copy link
Member

@mjsax mjsax Mar 14, 2026

Choose a reason for hiding this comment

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

I don't think we need to have this as a member, but KeyValueStoreFacade can just inherit from GenericReadOnlyKeyValueStoreFacade directly.

I am going to just push a change for this (also removing unnecessary tests -- don't need to re-test what we inherit from GenericReadOnlyKeyValueStoreFacade in KeyValueStoreFacadeTest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants