-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement time-based offset seeking across broker and client APIs #29
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
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -119,6 +119,26 @@ public LogSegment getSegmentForOffset(String topic, long offset) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Finds the first offset across all segments for a topic with timestamp >= targetTimestamp. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public long findOffsetByTimestamp(String topic, long targetTimestamp) throws IOException { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ConcurrentSkipListMap<Long, LogSegment> segments = topicSegments.get(topic); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (segments == null || segments.isEmpty()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Iterate through segments to find the first one that has the timestamp. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (LogSegment segment : segments.values()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| long offset = segment.findOffsetByTimestamp(targetTimestamp); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (offset != -1) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return offset; // Found it in this segment! | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+122
to
+141
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. 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift Optimize segment search to prevent O(N) disk scan. The current implementation linearly scans all segments from oldest to newest by calling You can optimize this by checking only the first message of each segment to find the exact segment that bounds the target timestamp, reducing the scan to just one segment. 🚀 Proposed optimization public long findOffsetByTimestamp(String topic, long targetTimestamp) throws IOException {
ConcurrentSkipListMap<Long, LogSegment> segments = topicSegments.get(topic);
if (segments == null || segments.isEmpty()) {
return -1;
}
- // Iterate through segments to find the first one that has the timestamp.
- for (LogSegment segment : segments.values()) {
- long offset = segment.findOffsetByTimestamp(targetTimestamp);
- if (offset != -1) {
- return offset; // Found it in this segment!
- }
- }
-
- return -1;
+ LogSegment candidate = null;
+ for (LogSegment segment : segments.values()) {
+ if (segment.getSize() == 0) continue;
+
+ try {
+ StoredMessage firstMsg = segment.read(0);
+ if (firstMsg.getTimestamp() >= targetTimestamp) {
+ if (candidate == null) {
+ return firstMsg.getOffset();
+ }
+ long offset = candidate.findOffsetByTimestamp(targetTimestamp);
+ return offset != -1 ? offset : firstMsg.getOffset();
+ }
+ } catch (CorruptRecordException e) {
+ // Ignore corruption at the start of a segment and fall back to tracking it
+ logger.warn("Corrupt first record in segment {}, skipping fast-path check", segment.getFilePath());
+ }
+ candidate = segment;
+ }
+
+ return candidate != null ? candidate.findOffsetByTimestamp(targetTimestamp) : -1;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public Map<String, ConcurrentSkipListMap<Long, LogSegment>> getAllSegments() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return topicSegments; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -225,6 +225,30 @@ public synchronized void truncate(long size) throws IOException { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Linearly scans the segment to find the first offset with timestamp >= targetTimestamp. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param targetTimestamp the timestamp to search for. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @return the offset, or -1 if no message in this segment matches. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public synchronized long findOffsetByTimestamp(long targetTimestamp) throws IOException { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| long position = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| long foundOffset = -1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (position < currentSize) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StoredMessage msg = read(position); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (msg.getTimestamp() >= targetTimestamp) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foundOffset = msg.getOffset(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| position += 4 + msg.getSerializedSize(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (CorruptRecordException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warn("Corrupt record found while searching segment {} at pos {}", filePath, position); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return foundOffset; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+228
to
+250
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. 🚀 Performance & Scalability | 🔴 Critical | ⚡ Quick win Remove
Since ⚡ Proposed fix- public synchronized long findOffsetByTimestamp(long targetTimestamp) throws IOException {
+ public long findOffsetByTimestamp(long targetTimestamp) throws IOException {
long position = 0;
long foundOffset = -1;
- while (position < currentSize) {
+ long sizeLimit = currentSize;
+ while (position < sizeLimit) {
try {
StoredMessage msg = read(position);
if (msg.getTimestamp() >= targetTimestamp) {
foundOffset = msg.getOffset();
break;
}
position += 4 + msg.getSerializedSize();
} catch (CorruptRecordException e) {
logger.warn("Corrupt record found while searching segment {} at pos {}", filePath, position);
break;
}
}
return foundOffset;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void close() throws IOException { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (fileChannel.isOpen()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -315,6 +315,66 @@ public void subscribe(String topic, long fromOffset) throws IOException { | |
| logger.info("Subscribed to topic '{}' from explicit offset {} (group='{}')", topic, fromOffset, consumerGroup); | ||
| } | ||
|
|
||
| /** | ||
| * Subscribe to a topic, starting from the first message at or after the given timestamp. | ||
| * Overrides the current offset for the topic. | ||
| */ | ||
| public void seekByTime(String topic, long timestamp) throws IOException { | ||
| long offset = doSearchOffsetByTime(topic, timestamp); | ||
| if (offset >= 0) { | ||
| topicOffsets.put(topic, offset); | ||
| if (groupMode) { | ||
| commit(topic, offset); | ||
| } | ||
| logger.info("Subscribed to topic '{}' from time-based offset {} (timestamp={})", topic, offset, timestamp); | ||
| } else { | ||
| logger.warn("Could not find any offset for topic '{}' at or after timestamp {}", topic, timestamp); | ||
| } | ||
| } | ||
|
Comment on lines
+318
to
+333
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Topic isn't registered on a failed seek, unlike
🤖 Prompt for AI Agents |
||
|
|
||
| private long doSearchOffsetByTime(String topic, long timestamp) throws IOException { | ||
| int attempts = 0; | ||
| while (attempts < MAX_RETRIES) { | ||
| try { | ||
| ensureConnectedWithRetry(); | ||
| SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.newBuilder() | ||
| .setTopic(topic) | ||
| .setTimestamp(timestamp) | ||
| .build(); | ||
|
|
||
| sendEnvelope(MessageEnvelope.newBuilder() | ||
| .setType(MessageType.SEARCH_OFFSET_BY_TIME_REQUEST) | ||
| .setPayload(request.toByteString()) | ||
| .build()); | ||
| MessageEnvelope responseEnvelope = receiveEnvelope(); | ||
|
|
||
| if (responseEnvelope.getType() == MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE) { | ||
| SearchOffsetByTimeResponse resp = SearchOffsetByTimeResponse.parseFrom(responseEnvelope.getPayload()); | ||
| return resp.getOffset(); | ||
| } else if (responseEnvelope.getType() == MessageType.PRODUCE_RESPONSE) { | ||
| ProduceResponse errorResp = ProduceResponse.parseFrom(responseEnvelope.getPayload()); | ||
| if (tryRedirectToLeader(errorResp.getErrorMessage())) { | ||
| attempts++; | ||
| continue; | ||
| } | ||
| throw new IOException("Error searching offset by time: " + errorResp.getErrorMessage()); | ||
| } else { | ||
| throw new IOException("Unexpected response type: " + responseEnvelope.getType()); | ||
| } | ||
| } catch (IOException e) { | ||
| if (e.getMessage() != null && e.getMessage().contains("Error searching offset")) { | ||
| throw e; | ||
| } | ||
| attempts++; | ||
| if (attempts >= MAX_RETRIES) { | ||
| throw new IOException("Failed to search offset by time after retries", e); | ||
| } | ||
| reconnect(); | ||
| } | ||
| } | ||
| return -1; | ||
| } | ||
|
|
||
| public List<ConsumedMessage> poll() throws IOException { | ||
| return poll(DEFAULT_MAX_MESSAGES, DEFAULT_POLL_TIMEOUT_MS); | ||
| } | ||
|
|
||
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add structural error handling and metrics telemetry.
Unlike other request handlers in this class (e.g.,
handleFetchOffsetRequest), this method lacks atry-catchblock to handle malformed payloads or runtime exceptions and does not record metrics. IfparseFromfails, the exception will propagate up to the channel, disconnecting the client instead of returning a well-formed error response.🛠️ Proposed fix to align with existing request handlers
📝 Committable suggestion
🤖 Prompt for AI Agents