Marton Greber 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: (5 comments) Thanks for taking a look! ParameterTool is already covered by tests in Flink: https://github.com/a0x8o/flink/tree/941b7259fe26a82908204f1b8d66d436357c8a66/flink-java/src/test/java/org/apache/flink/api/java/utils The rest of the open questions can be answered by examining the classes that ParameterTool relies on; I’ve added those findings to your comments. I decided not to add basic tests to our repository because they would introduce unnecessary bloat. 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 conf It will throw IllegalStateException.class with message "Key <key> should has only one value" [1] [1]: https://github.com/a0x8o/flink/blob/941b7259fe26a82908204f1b8d66d436357c8a66/flink-java/src/test/java/org/apache/flink/api/java/utils/MultipleParameterToolTest.java#L30-L66 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? In the end it uses Boolean.valueOf(String) , it only evaluates true if the argument is true, case insensitive. 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 'cl ParameterTool relies on Java’s Enum.valueOf(), which is case-sensitive. This requires exact match. 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 It is not expected, it will throw parsing error. 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 inva Yes there are. ParameterTools getLong() function uses -> Long.parseLong(value) which if not within bounds throws NumberFormatException Same for Int etc. -- 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: Wed, 30 Jul 2025 13:12:05 +0000 Gerrit-HasComments: Yes
