[GitHub] [kafka] hachikuji commented on a diff in pull request #13834: KAFKA-15076; KRaft should prefer latest snapshot

["hachikuji (via GitHub)" <gi...@apache.org>]

hachikuji commented on code in PR #13834:
URL: https://github.com/apache/kafka/pull/13834#discussion_r1224616596


##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -308,7 +308,12 @@ private void onUpdateLeaderHighWatermark(
     private void updateListenersProgress(long highWatermark) {
         for (ListenerContext listenerContext : listenerContexts.values()) {
             listenerContext.nextExpectedOffset().ifPresent(nextExpectedOffset -> {
-                if (nextExpectedOffset < log.startOffset() && nextExpectedOffset < highWatermark) {
+                // Send snapshot if the listener is at the beging of the log and there is a snapshot or

Review Comment:
   nit: beginning



##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -308,7 +308,12 @@ private void onUpdateLeaderHighWatermark(
     private void updateListenersProgress(long highWatermark) {
         for (ListenerContext listenerContext : listenerContexts.values()) {
             listenerContext.nextExpectedOffset().ifPresent(nextExpectedOffset -> {
-                if (nextExpectedOffset < log.startOffset() && nextExpectedOffset < highWatermark) {
+                // Send snapshot if the listener is at the beging of the log and there is a snapshot or
+                // the listener is trying to read an offset for which there isn't a segment in the log.
+                if (nextExpectedOffset < highWatermark &&
+                    ((nextExpectedOffset == 0 && latestSnapshot().isPresent()) ||

Review Comment:
   Not sure how useful the optimization is if it only covers the case at the beginning of the log. We could potentially send the snapshot any time its offset is higher than `nextExpectedOffset`? Perhaps there is a smarter heuristic to avoid fetching snapshots unnecessarily. Fetching the log is not super efficient I guess because of the healthcheck records.



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org