Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18604 )
Change subject: [Tools] Support to config hash bucket numbers when copy a table ...................................................................... Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@98 PS10, Line 98: keys dimension http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@98 PS10, Line 98: for every in each http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@435 PS10, Line 435: if (!partition_schema.hash_schema().empty()) What if the --create_table_hash_bucket_nums flag is specified for table with no hash partitioning? Should the tool report an error in that case instead of quietly ignoring that? http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@444 PS10, Line 444: // FLAGS_create_table_hash_bucket_nums is defined, then it's size : // must be equal to the number of hash dimension. nit: how about If the --create_table_hash_bucket_nums flag is set, the number of comma-separated elements must be equal to the number of hash schema dimensions. http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@447 PS10, Line 447: Specified hash bucket numbers " : "are not equal the number how about: count of hash bucket numbers must be equal to the number of hash schema dimensions http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@454 PS10, Line 454: digit Well, it seems safe_strto32 can accept numbers if hexadecimal notation as well, so this code will accept 0xff as well, if I'm not mistaken. Anyways, more actionable error would be: return Status::InvalidArgument(Substitute("'$0': cannot parse the number of hash buckets", hash_bucket_nums_str[i])); http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@454 PS10, Line 454: ! nit here and everywhere: there no need for exclamation marks in error messages. The exclamation marks don't add any actionable information but make error messages longer and harder to read. http://gerrit.cloudera.org:8080/#/c/18604/10/src/kudu/tools/table_scanner.cc@457 PS10, Line 457: Hash bucket number must be >= 2 how about: number of hash buckets must not be less than 2 -- To view, visit http://gerrit.cloudera.org:8080/18604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cec38e5ea09c66bfed20622b85033602da60d41 Gerrit-Change-Number: 18604 Gerrit-PatchSet: 10 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 30 Jun 2022 19:33:23 +0000 Gerrit-HasComments: Yes
