Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23121 )
Change subject: KUDU-3662 [4/n] Add reader & writer config parsing ...................................................................... Patch Set 3: Code-Review+2 (5 comments) Overall this patch LGTM. I posted several questions on the behavior of the config parser, but I guess it's more about the behavior of Flink config parser itself. If you think it makes sense to explicitly verify the questioned behavior in this context, maybe add corresponding test scenarios. Otherwise, feel free to submit this patch as-is once answering the questions in the review comments. Thanks! http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java File java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java: http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java@96 PS3, Line 96: "--reader.batchSizeBytes", "2048000", What if there are duplicates in the config for a parameter? Would the config be valid, and if yes, then what would be the result value for the parameter that were retrieved by the corresponding getXxx() method? http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java@99 PS3, Line 99: "--reader.prefetching", "true", Just curious: is the setting for this parameter is case-sensitive or not? Also, would '1' be converted into 'true' or it would be an invalid value? http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java@101 PS3, Line 101: CLOSEST_REPLICA Just curious: is this parameter case-sensitive or not? E.g., if set to 'closest_replica', would the parsed result be the same when it's set to 'CLOSEST_REPLICA'? http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java@143 PS3, Line 143: 60000 Is it always expected to have numbers in decimal notation, or it recognizes and accepts values in hexadecimal as well? For example, what if this were '0x1000' instead of '60000'? http://gerrit.cloudera.org:8080/#/c/23121/3/java/kudu-replication/src/test/java/org/apache/kudu/replication/TestReplicationConfigParser.java@144 PS3, Line 144: 5242880 What if setting this to something higher than 2^31 -- would that be an invalid value or it'd be accepted? If accepted, would it work as expected? BTW, are there 'no limit' semantics for this parameter? -- To view, visit http://gerrit.cloudera.org:8080/23121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48633a52046c7b5e637786d8e3c72d89946dc3e9 Gerrit-Change-Number: 23121 Gerrit-PatchSet: 3 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: 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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 29 Jul 2025 15:39:19 +0000 Gerrit-HasComments: Yes
