[GitHub] [kafka] C0urante commented on a diff in pull request #13367: KAFKA-14797: Emit offset sync when offset translation lag would exceed max.offset.lag

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

C0urante commented on code in PR #13367:
URL: https://github.com/apache/kafka/pull/13367#discussion_r1134035661


##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceTask.java:
##########
@@ -316,13 +315,14 @@ static class PartitionState {
 
         // true if we should emit an offset sync
         boolean update(long upstreamOffset, long downstreamOffset) {
-            long upstreamStep = upstreamOffset - lastSyncUpstreamOffset;
-            long downstreamTargetOffset = lastSyncDownstreamOffset + upstreamStep;
+            // This value is what OffsetSyncStore::translateOffsets would compute for this offset given the last sync.
+            // Because this method is called at most once for each upstream offset, simplify upstreamStep to 1.
+            // TODO: share common implementation to enforce this relationship
+            long downstreamTargetOffset = lastSyncDownstreamOffset + 1;

Review Comment:
   I've started to like the name `downstreamTargetOffset` less and less, and the need for 2-3 lines of comments to explain what it's supposed to represent might hint that a better name could help more here.
   
   We can remove this variable entirely and just inline the value `lastSyncDownstreamOffset + 1` in its place, with a comment explaining the `+ 1` part. Or we could rename to something like `translatedLastDownstreamOffset`.



-- 
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