apoorvmittal10 commented on code in PR #22479:
URL: https://github.com/apache/kafka/pull/22479#discussion_r3412137054
##########
server/src/main/java/org/apache/kafka/server/share/dlq/ShareGroupDLQStateManager.java:
##########
@@ -653,6 +686,73 @@ private void handleProduceResponse(ClientResponse
response) {
requestErrorResponse(clientResponseError.exception());
}
}
+
+ private Map<Long, Record> maybeFetchRecordData() {
+ if
(!cacheHelper.isShareGroupDlqCopyRecordEnabled(param.groupId())) {
+ return Map.of();
+ }
+ long startTime = time.hiResClockMs();
+ TopicIdPartition tp = param.topicIdPartition();
+
+ FetchParams fetchParams = new FetchParams(
+ FetchRequest.CONSUMER_REPLICA_ID, // -1, reading as
a consumer
+ -1, // replicaEpoch
+ 0L, // maxWaitMs -
don't block
+ 1, // minBytes
+ DLQ_MAX_FETCH_BYTES, // maxBytes
+ FetchIsolation.HIGH_WATERMARK, // committed only
+ Optional.empty() // clientMetadata
+ );
+
+ long nextOffset = param.firstOffset();
+ long endOffset = param.lastOffset();
+ int recordCount = (int) (param.lastOffset() - param.firstOffset()
+ 1);
+
+ Map<Long, Record> recordMap = new HashMap<>(recordCount);
+ LinkedHashMap<TopicIdPartition, Long> offsets = new
LinkedHashMap<>();
+ LinkedHashMap<TopicIdPartition, Integer> maxBytesMap = new
LinkedHashMap<>();
+ maxBytesMap.put(tp, DLQ_MAX_FETCH_BYTES);
+
+ // We are fetching data for one TopicIdPartition only. Hence, there
+ // is no need to keep recreating the maxBytes map, and we can
re-use a
+ // single copy. In similar vein, we needn't clear the offsets map
+ // either and just update the value corresponding to the
TopicIdPartition
+ // key in offsets map within the while loop.
+ while (nextOffset <= endOffset) {
+ offsets.put(tp, nextOffset);
+
+ LinkedHashMap<TopicIdPartition, LogReadResult> result =
+ logReader.read(fetchParams, Set.of(tp), offsets,
maxBytesMap);
+
+ LogReadResult res = result.get(param.topicIdPartition());
+ if (res == null || res.error().code() != Errors.NONE.code()) {
+ log.warn("Unable to fetch actual record at offset {} for
handler {}.", nextOffset, this);
+ return Map.of();
+ }
+
+ res.info().delayedRemoteStorageFetch.ifPresent(data ->
log.info(
+ "Some offset data in is in remote storage. Skipping it."));
+
+ boolean continueFetch = false;
+ for (RecordBatch batch : res.info().records.batches()) {
+ for (Record record : batch) {
+ if (record.offset() < param.firstOffset()) continue;
+ if (record.offset() > param.lastOffset()) {
+ log.trace("Preempted log fetch took {} ms for {}
records starting at {} for {}", time.hiResClockMs() - startTime,
+ recordCount, param.firstOffset(), this);
+ return Collections.unmodifiableMap(recordMap);
+ }
+ recordMap.put(record.offset(), record);
+ nextOffset = record.offset() + 1;
+ continueFetch = true;
+ }
+ }
+ if (!continueFetch) break; // no more records available
(reached HWM/LEO)
+ }
+ log.trace("Full log fetch took {} ms for {} records starting at {}
for {}", time.hiResClockMs() - startTime,
+ recordCount, param.firstOffset(), this);
+ return Collections.unmodifiableMap(recordMap);
+ }
Review Comment:
The info log is not always needed I think but only when the size is not
same. Currently it prints on every DLQ record which is not adding much value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
