Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17657 )
Change subject: [client] KUDU-2671 custom hash buckets API for table creation ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/17657/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17657/6//COMMIT_MSG@10 PS6, Line 10: creating > nit: to create Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.h@1261 PS6, Line 1261: bucketing. > nit: bucket schema. Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.h@1264 PS6, Line 1264: scheme different from the scheme > nit: schema instead of scheme Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.cc@953 PS6, Line 953: has_range_with_custom_hash_schema > nit: can break early after this bool is set to true Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.cc@958 PS6, Line 958: simplitity > nit: simplicity Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/client.cc@961 PS6, Line 961: splits > nit: split rows Done http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/17657/6/src/kudu/client/flex_partitioning_client-test.cc@200 PS6, Line 200: // One-level hash bucket structure with hashing on non-key column only: > Aren't hash bucket schemas only permitted to be on any subset of primary ke Yeah, as you can see it's the table is created without any errors, and it even contains the required number of replicas :) Apparently, a check is missing at the server side. Not sure whether we want to put a check into the client side as well, but a check at the server side is a must. What do you think if I add a check in a follow-up patch and turn corresponding test scenarios into negative ones, checking for proper error status? -- To view, visit http://gerrit.cloudera.org:8080/17657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98fd9754db850dcdd00a00738f470673f42ac5b4 Gerrit-Change-Number: 17657 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Fri, 16 Jul 2021 17:22:00 +0000 Gerrit-HasComments: Yes
