Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/23607 )
Change subject: KUDU-3662: Fix race condition in Kudu replication ...................................................................... Patch Set 2: (24 comments) few nits http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/ReplicationEnvProvider.java File java/kudu-replication/src/main/java/org/apache/kudu/replication/ReplicationEnvProvider.java: http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/ReplicationEnvProvider.java@29 PS2, Line 29: import org.apache.flink.connector.kudu.connector.writer.KuduOperationMapper; unused import http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/ReplicationEnvProvider.java@95 PS2, Line 95: setRestartStrategy it seems that this method is already deprecated and slated to be removed in future releases http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/ReplicationEnvProvider.java@95 PS2, Line 95: RestartStrategies also deprecated http://gerrit.cloudera.org:8080/#/c/23607/2/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/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/MetricWrappedKuduEnumerator.java@137 PS2, Line 137: @SuppressWarnings("unchecked") this suppression seems unnecessary, there are no lambdas here either, these are method references I ran a quick build with this suppression removed and no warnings were thrown http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/StatelessKuduReaderWrapper.java File java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/StatelessKuduReaderWrapper.java: http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/main/java/org/apache/kudu/replication/wrappedsource/StatelessKuduReaderWrapper.java@24 PS2, Line 24: import org.apache.flink.api.connector.source.SourceReaderContext; unused import http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java File java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java: http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@88 PS2, Line 88: 500 Maybe this needs to be a constant somewhere. Is it possible that the 100 set at #267 introduces some potential flakiness? http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@97 PS2, Line 97: KuduReaderConfig readerConfig = KuduReaderConfig.Builder : .setMasters(sourceHarness.getMasterAddressesAsString()) : .build(); : return readerConfig; : } : : protected KuduWriterConfig createDefaultWriterConfig() { : KuduWriterConfig writerConfig = KuduWriterConfig.Builder : .setMasters(sinkHarness.getMasterAddressesAsString()) : .build(); : return writerConfig; outside the scope of this change, but the writer/readerConfig could be inlined here, simply use return Kudu[Writer|Reader]Config.Builder.... http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@183 PS2, Line 183: throws Exception no Exception is thrown in this method, only AssertionErrors, either let's remove the throws clause here or change the AssertionErrors to IllegalArgumentException (this part needs updating this way too) http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@190 PS2, Line 190: chk- is this a Flink convention? would it make sense to extract this to a string constant? http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@197 PS2, Line 197: _metadata same question as above, would it make sense to make this a constant? http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@221 PS2, Line 221: 500 what happens if timeoutMillis is < 500? iiuc the check will not happen, right? http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/ReplicationTestBase.java@267 PS2, Line 267: 100 same question as at #221 http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java File java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java: http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@18 PS2, Line 18: import static org.junit.Assert.assertEquals; unused import http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@25 PS2, Line 25: import java.util.ArrayList; unused import http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@66 PS2, Line 66: MetricWrappedKuduEnumerator enumerator = createTestEnumerator(); MetricWrappedKuduEnumerator is Autocloseable, you could use try-with-resources http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@107 PS2, Line 107: MetricWrappedKuduEnumerator enumerator = createTestEnumerator(); MetricWrappedKuduEnumerator is Autocloseable, you could use try-with-resources http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@142 PS2, Line 142: pendingAfterCleanup1 nit: there is no pendingAfterCleanup2 so the 1 seems redundant :) http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@153 PS2, Line 153: PASSING I think this should be FAILING, since the current pass confirms the existence of the problem http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@158 PS2, Line 158: KuduSourceEnumerator plainEnumerator = createPlainEnumerator(); KuduSourceEnumerator is Autocloseable, you could use try-with-resources http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@189 PS2, Line 189: throws Exception Exception is never thrown by this method http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@217 PS2, Line 217: throws Exception Exception is never thrown by this method http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestMetricWrappedKuduEnumerator.java@238 PS2, Line 238: throws Exception Exception is never thrown by this method http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationCheckpoint.java File java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationCheckpoint.java: http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationCheckpoint.java@29 PS2, Line 29: import org.apache.flink.connector.kudu.connector.writer.KuduWriterConfig; unused import http://gerrit.cloudera.org:8080/#/c/23607/2/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationCheckpoint.java@83 PS2, Line 83: throws Exception Exception is never thrown in this method -- To view, visit http://gerrit.cloudera.org:8080/23607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I605a914aaa86b1bdf47537a5b21789de27972add Gerrit-Change-Number: 23607 Gerrit-PatchSet: 2 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 03 Nov 2025 13:40:10 +0000 Gerrit-HasComments: Yes
