Marton Greber 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: (23 comments) 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 Done 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 Done 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 Done 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 Done 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 se Yes absolutely. Moreover then we can reuse this value in waitForCheckpointCompletion (as we have the 500 there as well. basically while we wait for checkpoint completion using the default checkpoint interval for sleep is a good value) Regarding #267 waitForJobTermination: It is unrelated, every time we have to check the checkpoint presence in tests we can call waitForCheckpointCompletion. waitForJobTermination is added if the end status of the job is to be checked. (for example on an expected failure) 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 inli Done 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 r Done 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 This is Flink runtime naming. Extracted this into a 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? This is Flink runtime naming. Extracted this into 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, rig added a check that if a timeout is given that is smaller than the sleep value we throw exception 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 added same type of preliminary check. 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 Done 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 Done 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-resour Done 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-resour Done 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 :) Done 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 existen Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Done -- 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: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 07 Nov 2025 16:54:23 +0000 Gerrit-HasComments: Yes
