Skip to content

Comments

KAFKA-20134: Implement TimestampedWindowStoreWithHeaders (1/N)#21465

Merged
mjsax merged 7 commits intoapache:trunkfrom
frankvicky:KAFKA-20134-1-over-20132-2
Feb 20, 2026
Merged

KAFKA-20134: Implement TimestampedWindowStoreWithHeaders (1/N)#21465
mjsax merged 7 commits intoapache:trunkfrom
frankvicky:KAFKA-20134-1-over-20132-2

Conversation

@frankvicky
Copy link
Contributor

@frankvicky frankvicky commented Feb 12, 2026

This PR adds TimestampedSegmentWithHeaders,
TimestampedSegmentsWithHeaders and the corresponding unit tests for
the TimestampedWindowStoreWithHeaders introduced in KIP-1271.

Reviewers: Alieh Saeedi asaeedi@confluent.io, Matthias J. Sax
matthias@confluent.io

@github-actions github-actions bot added triage PRs from the community streams labels Feb 12, 2026
@frankvicky
Copy link
Contributor Author

This PR shouldn't be merge before #21446

@mjsax mjsax added kip Requires or implements a KIP ci-approved and removed triage PRs from the community labels Feb 12, 2026
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This calss does not need reviewing as it is being reviewd here: #21446

import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class TimestampedSegmentsWithHeadersTest {
Copy link
Contributor

@aliehsaeedii aliehsaeedii Feb 13, 2026

Choose a reason for hiding this comment

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

@frankvicky wondering why you pushed this class. Maybe skip pushing classes that are already pushed by other PRs. Just keep them local.

Copy link
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.

Overall LG. Thanks.

class TimestampedSegmentWithHeaders extends RocksDBTimestampedStoreWithHeaders
implements Comparable<TimestampedSegmentWithHeaders>, Segment {

public final long id;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the id public?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a public getter instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I follow the current pattern of TimestampedSegment.
I suppose this should be fine since the class is package-private and the field is final.

import org.apache.kafka.streams.state.internals.metrics.RocksDBMetricsRecorder;

/**
* Manages the {@link TimestampedSegmentWithHeaders}s that are used by the * {@link RocksDBTimestampedSegmentedBytesStoreWithHeaders}.
Copy link
Contributor

Choose a reason for hiding this comment

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

is the * before {@link RocksDBTimestampedSegmentedBytesStoreWithHeaders}. needed?

@frankvicky frankvicky requested a review from mjsax February 16, 2026 14:27
@frankvicky frankvicky force-pushed the KAFKA-20134-1-over-20132-2 branch from a49f66e to 1968e40 Compare February 18, 2026 15:25
@mjsax mjsax merged commit da0c207 into apache:trunk Feb 20, 2026
24 checks passed
@mjsax
Copy link
Member

mjsax commented Feb 20, 2026

Thanks for the PR. Merged to trunk.

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.

3 participants