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 5: Code-Review+2 (4 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(); : } > Yeah, but that's super slow -- even with 10ms tablet server to master hearb Yep, sgtm. Makes sense, given this is all actively being developed. 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@260 PS3, Line 260: InsertOrDie(&dict, it->lower, std::distance(ranges_ptr->cbegin(), it)); : } : } : if (range_bounds.size() != ranges_ptr->size()) { > Ah, good catch -- not sure how I missed this. Ack http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@1525 PS3, Line 1525: > Yeah, that would make a very long string. I think I'd rather revamp the te Sounds good. I agree it makes sense to consolidate terminology, especially if it helps reduce length without sacrificing clarity. 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) > It might be an option and I started with that (well, I did use iterators to Ah I see. Yeah I'm fine with leaving this as is for this patch. -- 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: 5 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 20:21:32 +0000 Gerrit-HasComments: Yes
