Zoltan Chovan 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 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/23246/1/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/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@71 PS1, Line 71: y { : this.lastEndTimestampField = delegate.getClass().getDeclaredField("lastEndTimestamp"); : this.lastEndTimestampField.setAccessible(true); : : this.pendingField = delegate.getClass().getDeclaredField("pending"); : this.pendingField.setAccessible(true); : : this.unassignedField = delegate.getClass().getDeclaredField("unassigned"); : this.unassignedField.setAccessible(true); why not use method (getPrivateField) similar in MetricWrappedKuduSource? maybe extract that method somewhere so it could be reused? http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@90 PS1, Line 90: () -> getLastEndTimestamp() this could be replaced with a method reference, e.g.: context.metricGroup().gauge("lastEndTimestamp", (Gauge<Long>) this::getLastEndTimestamp); same for the others http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@136 PS1, Line 136: if (valObj instanceof Long) { : long hybridTime = (Long) valObj; This and the following parts are a bit weird. First you check if valObj is an instance of Long, then you explicitly cast it as Long but in a variable that is not a Long but a long (primitive vs Java class) Also at the end the original value which was a Long is returned as a long, so we're potentially losing information (e.g. a Long can be null, a long must have a value, what happens if valObj is null?), but this is all moot because the original value is a primitive long in KuduSourceEnumerator :) tl;dr please clean up the Long/long casts http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@155 PS1, Line 155: List here and in the next method, using raw types is usually discuraged [0], since we do know the actual types of the list items we can use those types explicitly, there is no real "benefit" but it would be a more readable format, e.g. return ((List<KuduSourceSplit>) valObj).size(); Also why not just simply cast it as you access the value? The whole thing would become much simpler: try { List<KuduSourceSplit> pending = (List<KuduSourceSplit>) pendingField.get(delegate); return pending.size(); } catch (IllegalAccessException | IllegalArgumentException e) { throw new RuntimeException("Failed to access 'pending' field reflectively", e); } [0] https://www.baeldung.com/raw-types-java http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduSource.java File java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduSource.java: http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduSource.java@54 PS1, Line 54: // We use reflection here for convenience - ideally we could plumb configs in the maybe leave a todo() comment here as well just as a reminder http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduSource.java@70 PS1, Line 70: type this parameter is never used, removable? http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduSource.java@97 PS1, Line 97: throws Exception This can be removed as this Exception is never thrown. The NoSuchFieldException is caught in MetricWrappedKuduEnumerator's constructor http://gerrit.cloudera.org:8080/#/c/23246/1/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/1/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java@60 PS1, Line 60: info here and at other places you could used parameterised strings instead of concatenating, e.g. LOG.info("Timestamp advanced from {} to {} (waited {}ms)", previousTimestamp, currentTimestamp, waitedMs); http://gerrit.cloudera.org:8080/#/c/23246/1/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationMetrics.java@78 PS1, Line 78: if (lastEndTimestamp.isPresent()) { : return lastEndTimestamp.get().getValue(); : } : return null; could be simplified as: return lastEndTimestamp.map(Gauge::getValue).orElse(null); -- 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: 1 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 06 Aug 2025 09:03:47 +0000 Gerrit-HasComments: Yes
