feat: implement time-based offset seeking across broker and client APIs#29
Conversation
📝 WalkthroughWalkthroughAdds timestamp-based offset lookup to the protocol and broker, exposes ChangesTimestamp-based offset seeking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Broker
participant LogStore
participant ConsumerGroup
Consumer->>Broker: Request offset for topic and timestamp
Broker->>LogStore: Search records for timestamp
LogStore-->>Broker: Return first matching offset
Broker-->>Consumer: Return timestamp offset
Consumer->>ConsumerGroup: Commit offset when group mode is enabled
Consumer->>Consumer: Update local topic offset
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
drmq-python-client/drmq_client.py (1)
384-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStray
print()alongside the raised exception.The rest of the file logs via
logger.warning/logger.error; this new branch uses a rawprint(...)in addition to raising. Preferlogger.error(...)for consistency and to avoid unstructured output to stdout.♻️ Proposed fix
else: - print(f"POLL ERROR from broker: {resp.error_message}") + logger.error(f"Poll failed: {resp.error_message}") raise DRMQConnectionError(f"Poll failed: {resp.error_message}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drmq-python-client/drmq_client.py` around lines 384 - 386, Replace the raw print in the poll error branch with logger.error, preserving the existing broker error message and DRMQConnectionError raise in the surrounding polling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drmq-broker/src/main/java/com/drmq/broker/ClientHandler.java`:
- Around line 484-498: Update handleSearchOffsetByTimeRequest to match the
structural error handling and metrics telemetry used by handlers such as
handleFetchOffsetRequest: wrap payload parsing and offset lookup in the
established try-catch flow, record the corresponding request success/failure
metrics, and return the standard well-formed error response for malformed
payloads or runtime exceptions instead of propagating them to the channel.
In `@drmq-broker/src/main/java/com/drmq/broker/persistence/LogManager.java`:
- Around line 122-141: Update LogManager.findOffsetByTimestamp to avoid calling
LogSegment.findOffsetByTimestamp across every segment. Inspect each segment’s
first-message timestamp to identify the segment whose range bounds
targetTimestamp, then invoke findOffsetByTimestamp only on that segment;
preserve -1 for missing or out-of-range topics.
In `@drmq-broker/src/main/java/com/drmq/broker/persistence/LogSegment.java`:
- Around line 228-250: Remove synchronized from LogSegment.findOffsetByTimestamp
so the linear scan does not block append or appendBatch. Capture the volatile
currentSize once at method entry and use that local snapshot as the scan’s upper
bound, while preserving the existing read, corruption handling, and
offset-search behavior.
In `@drmq-client/src/main/java/com/drmq/client/DRMQConsumer.java`:
- Around line 318-333: The seekByTime method must register the topic even when
doSearchOffsetByTime returns no matching offset. Preserve a valid found offset,
but on failure add the topic using its prior offset when available or the
default offset 0, so doPoll continues to process it consistently with subscribe
and the other clients.
---
Nitpick comments:
In `@drmq-python-client/drmq_client.py`:
- Around line 384-386: Replace the raw print in the poll error branch with
logger.error, preserving the existing broker error message and
DRMQConnectionError raise in the surrounding polling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e84fe451-b3e2-49d2-bb65-9b56fd579ebf
📒 Files selected for processing (12)
drmq-broker/src/main/java/com/drmq/broker/ClientHandler.javadrmq-broker/src/main/java/com/drmq/broker/MessageStore.javadrmq-broker/src/main/java/com/drmq/broker/persistence/LogManager.javadrmq-broker/src/main/java/com/drmq/broker/persistence/LogSegment.javadrmq-client/src/main/java/com/drmq/client/DRMQConsumer.javadrmq-client/src/main/java/com/drmq/client/commandLineExample/ConsumerApp.javadrmq-integration-tests/src/test/java/com/drmq/integration/RaftIntegrationTest.javadrmq-protocol/src/main/proto/messages.protodrmq-python-client/drmq_client.pydrmq-python-client/messages_pb2.pydrmq-ts-client/src/client.tsdrmq-ts-client/src/messages.ts
| private MessageEnvelope handleSearchOffsetByTimeRequest(MessageEnvelope envelope) throws IOException { | ||
| SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.parseFrom(envelope.getPayload()); | ||
|
|
||
| long offset = messageStore.findOffsetByTimestamp(request.getTopic(), request.getTimestamp()); | ||
|
|
||
| SearchOffsetByTimeResponse.Builder response = SearchOffsetByTimeResponse.newBuilder() | ||
| .setSuccess(true) | ||
| .setOffset(offset); | ||
|
|
||
| return MessageEnvelope.newBuilder() | ||
| .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE) | ||
| .setPayload(response.build().toByteString()) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 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 a try-catch block to handle malformed payloads or runtime exceptions and does not record metrics. If parseFrom fails, 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
- private MessageEnvelope handleSearchOffsetByTimeRequest(MessageEnvelope envelope) throws IOException {
- SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.parseFrom(envelope.getPayload());
-
- long offset = messageStore.findOffsetByTimestamp(request.getTopic(), request.getTimestamp());
-
- SearchOffsetByTimeResponse.Builder response = SearchOffsetByTimeResponse.newBuilder()
- .setSuccess(true)
- .setOffset(offset);
-
- return MessageEnvelope.newBuilder()
- .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE)
- .setPayload(response.build().toByteString())
- .build();
- }
+ private MessageEnvelope handleSearchOffsetByTimeRequest(MessageEnvelope envelope) throws IOException {
+ long startNanos = System.nanoTime();
+ try {
+ SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.parseFrom(envelope.getPayload());
+
+ long offset = messageStore.findOffsetByTimestamp(request.getTopic(), request.getTimestamp());
+
+ SearchOffsetByTimeResponse.Builder response = SearchOffsetByTimeResponse.newBuilder()
+ .setSuccess(true)
+ .setOffset(offset);
+
+ BrokerMetrics.get().recordRequest("search_offset_by_time", true,
+ System.nanoTime() - startNanos, 0, 0);
+
+ return MessageEnvelope.newBuilder()
+ .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE)
+ .setPayload(response.build().toByteString())
+ .build();
+ } catch (Exception e) {
+ logger.error("Error searching offset by time", e);
+ BrokerMetrics.get().recordRequest("search_offset_by_time", false,
+ System.nanoTime() - startNanos, 0, 0);
+ SearchOffsetByTimeResponse response = SearchOffsetByTimeResponse.newBuilder()
+ .setSuccess(false)
+ .setOffset(-1)
+ .setErrorMessage(e.getMessage() != null ? e.getMessage() : "Unknown error")
+ .build();
+ return MessageEnvelope.newBuilder()
+ .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE)
+ .setPayload(response.toByteString())
+ .build();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private MessageEnvelope handleSearchOffsetByTimeRequest(MessageEnvelope envelope) throws IOException { | |
| SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.parseFrom(envelope.getPayload()); | |
| long offset = messageStore.findOffsetByTimestamp(request.getTopic(), request.getTimestamp()); | |
| SearchOffsetByTimeResponse.Builder response = SearchOffsetByTimeResponse.newBuilder() | |
| .setSuccess(true) | |
| .setOffset(offset); | |
| return MessageEnvelope.newBuilder() | |
| .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE) | |
| .setPayload(response.build().toByteString()) | |
| .build(); | |
| } | |
| private MessageEnvelope handleSearchOffsetByTimeRequest(MessageEnvelope envelope) throws IOException { | |
| long startNanos = System.nanoTime(); | |
| try { | |
| SearchOffsetByTimeRequest request = SearchOffsetByTimeRequest.parseFrom(envelope.getPayload()); | |
| long offset = messageStore.findOffsetByTimestamp(request.getTopic(), request.getTimestamp()); | |
| SearchOffsetByTimeResponse.Builder response = SearchOffsetByTimeResponse.newBuilder() | |
| .setSuccess(true) | |
| .setOffset(offset); | |
| BrokerMetrics.get().recordRequest("search_offset_by_time", true, | |
| System.nanoTime() - startNanos, 0, 0); | |
| return MessageEnvelope.newBuilder() | |
| .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE) | |
| .setPayload(response.build().toByteString()) | |
| .build(); | |
| } catch (Exception e) { | |
| logger.error("Error searching offset by time", e); | |
| BrokerMetrics.get().recordRequest("search_offset_by_time", false, | |
| System.nanoTime() - startNanos, 0, 0); | |
| SearchOffsetByTimeResponse response = SearchOffsetByTimeResponse.newBuilder() | |
| .setSuccess(false) | |
| .setOffset(-1) | |
| .setErrorMessage(e.getMessage() != null ? e.getMessage() : "Unknown error") | |
| .build(); | |
| return MessageEnvelope.newBuilder() | |
| .setType(MessageType.SEARCH_OFFSET_BY_TIME_RESPONSE) | |
| .setPayload(response.toByteString()) | |
| .build(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drmq-broker/src/main/java/com/drmq/broker/ClientHandler.java` around lines
484 - 498, Update handleSearchOffsetByTimeRequest to match the structural error
handling and metrics telemetry used by handlers such as
handleFetchOffsetRequest: wrap payload parsing and offset lookup in the
established try-catch flow, record the corresponding request success/failure
metrics, and return the standard well-formed error response for malformed
payloads or runtime exceptions instead of propagating them to the channel.
| /** | ||
| * 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 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 segment.findOffsetByTimestamp(targetTimestamp). If a topic has many segments and the target timestamp is recent, this will read and parse every historical message from disk, causing severe I/O amplification and potentially blocking the thread for seconds or minutes.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 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; | |
| } | |
| /** | |
| * 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; | |
| } | |
| 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; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drmq-broker/src/main/java/com/drmq/broker/persistence/LogManager.java` around
lines 122 - 141, Update LogManager.findOffsetByTimestamp to avoid calling
LogSegment.findOffsetByTimestamp across every segment. Inspect each segment’s
first-message timestamp to identify the segment whose range bounds
targetTimestamp, then invoke findOffsetByTimestamp only on that segment;
preserve -1 for missing or out-of-range topics.
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔴 Critical | ⚡ Quick win
Remove synchronized to prevent blocking concurrent appends.
findOffsetByTimestamp linearly scans the segment, which can involve thousands of disk reads and Protobuf allocations. Holding the synchronized lock for the entire scan will block append and appendBatch operations on the active segment, causing severe latency spikes for producers.
Since read() uses position-based FileChannel.read() which is thread-safe, and currentSize is volatile, the scan can safely run concurrently without the lock. Capturing currentSize in a local variable at the start of the method also ensures a stable upper bound for the scan.
⚡ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 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; | |
| } | |
| /** | |
| * 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 long findOffsetByTimestamp(long targetTimestamp) throws IOException { | |
| long position = 0; | |
| long foundOffset = -1; | |
| 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; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drmq-broker/src/main/java/com/drmq/broker/persistence/LogSegment.java` around
lines 228 - 250, Remove synchronized from LogSegment.findOffsetByTimestamp so
the linear scan does not block append or appendBatch. Capture the volatile
currentSize once at method entry and use that local snapshot as the scan’s upper
bound, while preserving the existing read, corruption handling, and
offset-search behavior.
| /** | ||
| * 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Topic isn't registered on a failed seek, unlike subscribe() and the Python/TS equivalents.
topicOffsets.put(topic, offset) only runs when offset >= 0. If no matching offset is found, the topic is never added to topicOffsets, so doPoll() (which iterates topicOffsets.entrySet()) will silently skip it on subsequent poll() calls. subscribe() always registers the topic (even with a default), and the Python/TS seek_by_time/seekByTime unconditionally add the topic to subscriptions up front, falling back to offset 0 on the next poll if the seek didn't find a match. Consider registering the topic (e.g., at its prior offset or 0) even when no match is found, so behavior matches subscribe() and the other clients.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drmq-client/src/main/java/com/drmq/client/DRMQConsumer.java` around lines 318
- 333, The seekByTime method must register the topic even when
doSearchOffsetByTime returns no matching offset. Preserve a valid found offset,
but on failure add the topic using its prior offset when available or the
default offset 0, so doPoll continues to process it consistently with subscribe
and the other clients.
Summary by CodeRabbit
New Features
seektimewith Unix millisecond and ISO-8601 timestamps.Bug Fixes