chirag-wadhwa5 commented on code in PR #17573:
URL: https://github.com/apache/kafka/pull/17573#discussion_r1829056690
##########
core/src/main/java/kafka/server/share/ShareFetchUtils.java:
##########
@@ -128,4 +131,17 @@ static long offsetForEarliestTimestamp(TopicIdPartition
topicIdPartition, Replic
Optional.empty(), true).timestampAndOffsetOpt();
return timestampAndOffset.isEmpty() ? (long) 0 :
timestampAndOffset.get().offset;
}
+
+ /**
+ * The method is used to get the offset for the latest timestamp for the
topic-partition.
+ *
+ * @return The offset for the latest timestamp.
+ */
+ static long offsetForLatestTimestamp(TopicIdPartition topicIdPartition,
ReplicaManager replicaManager) {
+ // Isolation level is set to READ_UNCOMMITTED, matching with that used
in share fetch requests
+ Option<FileRecords.TimestampAndOffset> timestampAndOffset =
replicaManager.fetchOffsetForTimestamp(
+ topicIdPartition.topicPartition(),
ListOffsetsRequest.LATEST_TIMESTAMP, new
Some<>(IsolationLevel.READ_UNCOMMITTED),
+ Optional.empty(), true).timestampAndOffsetOpt();
+ return timestampAndOffset.isEmpty() ? (long) 0 :
timestampAndOffset.get().offset;
Review Comment:
Hi @apoorvmittal10 , when `replicaManager.fetchOffsetForTimestamp` is called
with `LATEST_TIMESTAMP`, as per the current code, only 2 things are possible,
either it will throw an error or it will return a populated response (not a
None object, which is possible when the same method is called with
`EARLIEST_TIMESTAMP`). Now the error handling is there for when an error is
thrown, but apart from that I don't suppose we need any other checks for
whether the offset value is populated or not. But, if we want to have this
check in place, I don't think we can assume any other value in place of this.
The only right way would be to fail the share partition initialization. Another
point to note is when `offsetForEarliestTimestamp` is called with
`EARLIEST_TIMESTAMP`, the current code would return 0 if the returned value
from `replicaManager.fetchOffsetForTimestamp` is None, which again would be a
wrong assumption in case the start offset of the log is beyond 0. I believe the
correct way is
to fail the share partition initialization here as well.
--
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]
