-
Notifications
You must be signed in to change notification settings - Fork 15.1k
KAFKA-13499: Avoid restoring outdated records #21779
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,9 @@ public class InMemorySessionStore implements SessionStore<Bytes, byte[]> { | |
| this.metricScope = metricScope; | ||
| this.position = Position.emptyPosition(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Add a space here |
||
| public long retentionPeriod() { | ||
| return retentionPeriod; | ||
| } | ||
|
|
||
| @Override | ||
| public String name() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
| import org.apache.kafka.clients.consumer.MockConsumer; | ||
| import org.apache.kafka.clients.consumer.OffsetAndMetadata; | ||
| import org.apache.kafka.clients.consumer.OffsetAndTimestamp; | ||
| import org.apache.kafka.clients.consumer.internals.AutoOffsetResetStrategy; | ||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.PartitionInfo; | ||
|
|
@@ -1430,6 +1431,51 @@ private void addRecords(final long messages, final TopicPartition topicPartition | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void shouldSeekByTimestampForWindowedStoreWithoutCheckpoint() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add tests for negative test cases, broker doesn't have info and returns |
||
| final long retentionMs = Duration.ofHours(2).toMillis(); | ||
| final long offsetForTimestamp = 42L; | ||
|
|
||
| // Use a MockConsumer subclass that supports offsetsForTimes | ||
| final MockConsumer<byte[], byte[]> timestampConsumer = new MockConsumer<>(AutoOffsetResetStrategy.EARLIEST.name()) { | ||
| @Override | ||
| public synchronized Map<TopicPartition, OffsetAndTimestamp> offsetsForTimes(final Map<TopicPartition, Long> timestampsToSearch) { | ||
| final Map<TopicPartition, OffsetAndTimestamp> result = new java.util.HashMap<>(); | ||
| for (final Map.Entry<TopicPartition, Long> entry : timestampsToSearch.entrySet()) { | ||
| result.put(entry.getKey(), new OffsetAndTimestamp(offsetForTimestamp, entry.getValue())); | ||
| } | ||
| return result; | ||
| } | ||
| }; | ||
|
|
||
| // Set up mocks - storeMetadata returns null offset (no checkpoint) and positive retentionPeriod | ||
| final StateStoreMetadata windowStoreMetadata = mock(StateStoreMetadata.class); | ||
| final ProcessorStateManager windowStateManager = mock(ProcessorStateManager.class); | ||
| final StateStore windowStore = mock(StateStore.class); | ||
| when(windowStoreMetadata.changelogPartition()).thenReturn(tp); | ||
| when(windowStoreMetadata.store()).thenReturn(windowStore); | ||
| when(windowStoreMetadata.offset()).thenReturn(null); | ||
| when(windowStoreMetadata.retentionPeriod()).thenReturn(retentionMs); | ||
| when(windowStore.name()).thenReturn(storeName); | ||
| when(windowStateManager.storeMetadata(tp)).thenReturn(windowStoreMetadata); | ||
| when(windowStateManager.taskType()).thenReturn(ACTIVE); | ||
|
|
||
| final TaskId taskId = new TaskId(0, 0); | ||
| when(windowStateManager.taskId()).thenReturn(taskId); | ||
|
|
||
| timestampConsumer.updateBeginningOffsets(Collections.singletonMap(tp, 0L)); | ||
| adminClient.updateEndOffsets(Collections.singletonMap(tp, 100L)); | ||
|
|
||
| final StoreChangelogReader reader = | ||
| new StoreChangelogReader(time, config, logContext, adminClient, timestampConsumer, callback, standbyListener); | ||
|
|
||
| reader.register(tp, windowStateManager); | ||
| reader.restore(Collections.singletonMap(taskId, mock(Task.class))); | ||
|
|
||
| // The consumer should be seeked to the offset returned by offsetsForTimes, not to the beginning | ||
| assertEquals(offsetForTimestamp, timestampConsumer.position(tp)); | ||
| } | ||
|
|
||
| private void assignPartition(final long messages, | ||
| final TopicPartition topicPartition) { | ||
| consumer.updatePartitions( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinking instead of the mulitple
instanceofchecks we could introduce an interfaceWithRetentionPeriod(the name is up for debate) containing the single methodretentionPeriodand have the store types here implement it. Since all of the instances here are internal API this is possble without requiring a KIP. So the multiple checks would go to oneThis would also provide the added benefit of automatically picking up any store needing/using a retention period.
\cc @mjsax @aliehsaeedii @lucasbru @frankvicky