Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17769 )
Change subject: KUDU-2671 key encoding for custom hash bucket schemas ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/17769/4/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/17769/4/src/kudu/client/flex_partitioning_client-test.cc@228 PS4, Line 228: shared_ptr<KuduTable> table; : ASSERT_OK(client_->OpenTable(table_name, &table)); : : vector<scoped_refptr<TabletInfo>> all_tablets_info; : { : auto* cm = cluster_->mini_master(0)->master()->catalog_manager(); : CatalogManager::ScopedLeaderSharedLock l(cm); : scoped_refptr<master::TableInfo> table_info; : ASSERT_OK(cm->GetTableInfo(table->id(), &table_info)); : table_info->GetAllTablets(&all_tablets_info); : } : vector<scoped_refptr<TabletReplica>> replicas; : for (const auto& tablet_info : all_tablets_info) { : for (auto i = 0; i < cluster_->num_tablet_servers(); ++i) { : scoped_refptr<TabletReplica> r; : ASSERT_TRUE(cluster_->mini_tablet_server(i)->server()-> : tablet_manager()->LookupTablet(tablet_info->id(), &r)); : replicas.emplace_back(std::move(r)); : } : } : : int64_t count = 0; : for (const auto& r : replicas) { : count += r->CountLiveRowsNoFail(); : } nit: would client_->GetTableStatistics() work instead? http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@1525 PS3, Line 1525: > Well, instead I suggest to treat the previous line same as if it were I see. I suppose an alternative would be const PartitionSchema::HashBucketSchemas& PartitionSchema::GetHashBucketSchemasForKey( const string& key) const { which is more in line with style elsewhere. That said, I don't feel strongly about it, given I agree the length of the types makes it tougher to read. http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@1528 PS3, Line 1528: > You are right: the comment for the method is incorrect -- that's the range Thanks for fixing that! It's much clearer now. http://gerrit.cloudera.org:8080/#/c/17769/4/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17769/4/src/kudu/common/partition.cc@260 PS4, Line 260: std::distance(ranges_ptr->cbegin(), it) Rather than pointing to an index, would it make sense to have the value be the corresponding RangeWithHashSchemas*? http://gerrit.cloudera.org:8080/#/c/17769/4/src/kudu/common/partition.cc@1547 PS4, Line 1547: key nit: range_key -- To view, visit http://gerrit.cloudera.org:8080/17769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81aa1c10998d88a1bd5314fc3132037b1c8bbe4b Gerrit-Change-Number: 17769 Gerrit-PatchSet: 4 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, 13 Aug 2021 00:37:46 +0000 Gerrit-HasComments: Yes
