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 1: (9 comments) Thanks for the valuable review Zoltan! In order to be compliant with SecurityManager I've added a util class ReflectionSecurityUtils. As I'm writing this it just occurred to me that there was a patch: "Add SecurityManagerCompatibility shim" [1] I think I will update the code to use that compatibility shim. https://github.com/apache/kudu/commit/586cfc20905df35f49541b53af4442736790b0c1#diff-b483e8ec47e92b99d67ec001ba8ac040581468c015ae3fa4377ef2346d4017e5 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? ma Here the idea was that we can be a bit more performant if we just set the fields accessible in the constructor and during metrics extraction we just get the field values (no need to make them accessible on each read). 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.: Done 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 Done 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], sin Done 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 added todos on the class level here and in the enumerator. 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? Done 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 NoSuchFieldExcep Done 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 co Done 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: Done -- 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: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 18 Aug 2025 16:34:58 +0000 Gerrit-HasComments: Yes
