Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/23246 )
Change subject: KUDU-3662 [5/n] Add metrics support to replication ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/23246/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23246/4//COMMIT_MSG@11 PS4, Line 11: number > nit: number of splits? Done http://gerrit.cloudera.org:8080/#/c/23246/4//COMMIT_MSG@23 PS4, Line 23: unit test > nit: unit tests? Done http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java File java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java: http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@74 PS4, Line 74: lastEndTimestamp > What are your thoughts on renaming this to lastEditTimestampMs for clarity thats a good point, however this class is just a wrapper I wouldnt do the renaming here, rather in the original class: https://github.com/apache/flink-connector-kudu/blob/eb13082fc75a0061a1605c92f536157b4ac8a5b0/flink-connector-kudu/src/main/java/org/apache/flink/connector/kudu/source/enumerator/KuduSourceEnumerator.java#L84 Created a Flink ticket to track these: FLINK-38316 Thanks for the good point. http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@127 PS4, Line 127: private long getLastEndTimestamp() { : long hybridTime = ReflectionSecurityUtils.getLongFieldValue(lastEndTimestampField, delegate); : long[] parsed = HybridTimeUtil.HTTimestampToPhysicalAndLogical(hybridTime); : long epochMicros = parsed[0]; : return epochMicros / 1_000; : } > I see that only the physical part of the timestamp is exposed via this metr The physical timestamp alone provides all the necessary information to understand replication progress, detect lag, and trigger appropriate alerts. Adding the logical component would not enhance monitoring capabilities but would add unnecessary complexity to metrics interpretation. Since the replication job uses diff scans that operate on time ranges for temporal coverage rather than fine-grained operation sequencing, the logical component (which handles sub-microsecond ordering) provides no additional value for progress monitoring. Let me know if this sounds okay with you? http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java File java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java: http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java@116 PS4, Line 116: > Quick question (nit) for here and below: is it OK not calling this if any o good catch, added @After cleanup rule. http://gerrit.cloudera.org:8080/#/c/23246/4/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java@129 PS4, Line 129: > Here the timeout is 15s, but below it's 10s. Why is the difference and whe Initially we can have more delay due to job startup and initial snapshot scan, so 15s provides extra buffer. Subsequent waits only need to account for the 2s discovery interval plus processing time, so 10s (5x margin) is sufficient for slow environments. -- To view, visit http://gerrit.cloudera.org:8080/23246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibfbd34c707e7539ee88863399ae3061683f8bb3b Gerrit-Change-Number: 23246 Gerrit-PatchSet: 5 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 03 Sep 2025 12:25:54 +0000 Gerrit-HasComments: Yes
