[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 31 Jul 2020 18:15:00 + Gerrit-HasComments: No
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Benchmarked with: $ KUDU_ALLOW_SLOW_TESTS=1 ./build/latest/bin/table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 Before: 74,787 req/sec After: 79,451 req/sec (1.06x) Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Reviewed-on: http://gerrit.cloudera.org:8080/16137 Tested-by: Alexey Serbin Reviewed-by: Alexey Serbin --- M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/auto_rebalancer.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 7 files changed, 98 insertions(+), 40 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 13 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has removed a vote on this change. Change subject: Optimize GetTableLocations for better arena use .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 12: Verified+1 unrelated test failure (TSAN configuration) in TabletServerDiskError/TabletServerDiskErrorITest.TestFailDuringScanWorkload/0 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 31 Jul 2020 16:51:28 + Gerrit-HasComments: No
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#12). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Benchmarked with: $ KUDU_ALLOW_SLOW_TESTS=1 ./build/latest/bin/table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 Before: 74,787 req/sec After: 79,451 req/sec (1.06x) Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/auto_rebalancer.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 7 files changed, 98 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/12 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 12 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#11). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Benchmarked with: $ KUDU_ALLOW_SLOW_TESTS=1 ./build/latest/bin/table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 Before: 74,787 req/sec After: 79,451 req/sec (1.06x) Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/master/auto_rebalancer.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 6 files changed, 93 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/11 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 11 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 10: ASAN-related failure seems relevant: it seems TSInfoDict-related memory is leaking with new changes. TSInfosDict is also used in AutoRebalancerTask::GetTabletLeader() (see auto_rebalancer.cc), so that dance with STLDeleteElements is necessary there as well. >From the other side, maybe it's worth introducing an additional constructor >for TSInfosDict or an additional class for TSInfosDict to be used outside of >the CatalogManager. = ==29702==ERROR: LeakSanitizer: detected memory leaks Direct leak of 658080 byte(s) in 9140 object(s) allocated from: #0 0x534188 in operator new(unsigned long) /home/jenkins-slave/workspace/kudu-master/0/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:99 #1 0x7f35d3eb564f in kudu::master::TSInfoPB* google::protobuf::Arena::CreateMessageInternal(google::protobuf::Arena*) /home/jenkins-slave/workspace/kudu-master/0/thirdparty/installed/uninstrumented/include/google/protobuf/arena.h:501:14 #2 0x7f35d3eb564f in kudu::master::TSInfoPB* google::protobuf::Arena::CreateMaybeMessage(google::protobuf::Arena*) /home/jenkins-slave/workspace/kudu-master/0/build/asan/src/kudu/master/master.pb.cc:24126:10 #3 0x7f35db7187f9 in kudu::master::TSInfoPB* google::protobuf::Arena::CreateMessage(google::protobuf::Arena*) /home/jenkins-slave/workspace/kudu-master/0/thirdparty/installed/uninstrumented/include/google/protobuf/arena.h:304:12 #4 0x7f35db7187f9 in kudu::master::CatalogManager::BuildLocationsForTablet(scoped_refptr const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*)::$_30::operator()() const /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/catalog_manager.cc:5040:29 #5 0x7f35db72e47c in kudu::master::CatalogManager::BuildLocationsForTablet(scoped_refptr const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*)::$_29::operator()() const /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/catalog_manager.cc:5066:30 #6 0x7f35db72dfc2 in std::pair, std::equal_to, google::libc_allocator_with_realloc > >::mapped_type* const, bool> ComputePairIfAbsentReturnAbsense, std::equal_to, google::libc_allocator_with_realloc > >, kudu::master::CatalogManager::BuildLocationsForTablet(scoped_refptr const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*)::$_29>(google::dense_hash_map, std::equal_to, google::libc_allocator_with_realloc > >*, google::dense_hash_map, std::equal_to, google::libc_allocator_with_realloc > >::key_type const&, kudu::master::CatalogManager::BuildLocationsForTablet(scoped_refptr const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*)::$_29) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/gutil/map-util.h:903:14 #7 0x7f35db7178d9 in kudu::master::CatalogManager::BuildLocationsForTablet(scoped_refptr const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/catalog_manager.cc:5061:25 #8 0x7f35db71939f in kudu::master::CatalogManager::GetTabletLocations(std::string const&, kudu::master::ReplicaTypeFilter, kudu::master::TabletLocationsPB*, kudu::master::CatalogManager::TSInfosDict*, boost::optional) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/catalog_manager.cc:5121:10 #9 0x7f35db685fa3 in kudu::master::AutoRebalancerTask::GetTabletLeader(std::string const&, std::string*, kudu::HostPort*) const /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/auto_rebalancer.cc:375:5 #10 0x7f35db686a86 in kudu::master::AutoRebalancerTask::CheckMoveCompleted(kudu::rebalance::Rebalancer::ReplicaMove const&, bool*) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/auto_rebalancer.cc:679:3 #11 0x7f35db684bce in kudu::master::AutoRebalancerTask::CheckReplicaMovesCompleted(std::vector >*) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/auto_rebalancer.cc:642:16 #12 0x7f35db67de81 in kudu::master::AutoRebalancerTask::RunLoop() /home/jenkins-slave/workspace/kudu-master/0/src/kudu/master/auto_rebalancer.cc:247:7 #13 0x7f35c5dacbba in kudu::Thread::SuperviseThread(void*) /home/jenkins-slave/workspace/kudu-master/0/src/kudu/util/thread.cc:674:3 #14 0x7f35ce792183 in start_thread /build/eglibc-SvCtMH/eglibc-2.19/nptl/pthread_create.c:312 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#9). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Benchmarked with: $ KUDU_ALLOW_SLOW_TESTS=1 ./build/latest/bin/table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 Before: 74,787 req/sec After: 79,451 req/sec (1.06x) Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 6 files changed, 46 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/9 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 9 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc@220 PS7, Line 220: CHECK > It's not about SIGABRT vs SIGSEGV, IIRC it's easier to debug CHECK failures Ah, indeed: SIGABRT gives nice stack traces in the logs, agree. You are right. -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 13 Jul 2020 17:01:46 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc@220 PS7, Line 220: CHECK > What's the benefit crashing with SIGABRT vs SIGSEGV? A better reputation o It's not about SIGABRT vs SIGSEGV, IIRC it's easier to debug CHECK failures as that will leave a log message while SIGSEGV will only leave a stack trace and a minidump and you need gdb and debug symbols to figure out why it crashed. I do agree that if this is a hot path and we have enough tests then it makes sense to only DCHECK though as in that case the benefits would likely not outweigh the costs. -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 8 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 Jul 2020 22:03:55 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has uploaded a new patch set (#8) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 6 files changed, 46 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/8 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 8 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc@220 PS7, Line 220: CHECK > isn't it better to crash with CHECK failure than with a SIGSEGV though? What's the benefit crashing with SIGABRT vs SIGSEGV? A better reputation or something? :) As for the downsides, this is an extra instruction in a relatively hot path. It seems this is a protection against a programming error, and DCHECK() is good enough way to catch those. -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 Jul 2020 19:41:36 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc@220 PS7, Line 220: CHECK > nit: I guess if registration is an empty std::unique_ptr wrapper, the proce isn't it better to crash with CHECK failure than with a SIGSEGV though? -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 Jul 2020 13:41:59 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16137/7//COMMIT_MSG Commit Message: PS7: I think you mentioned running Alexey's table_locations-itest benchmark; can you share some of the numbers here, if you have them? -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 09 Jul 2020 22:45:41 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: Would it make sense to tag this commit with KUDU-636? -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 09 Jul 2020 12:59:26 + Gerrit-HasComments: No
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: -Code-Review Whoops, forgot to mention one issue with TSInfosDict. TSInfosDict is also used in AutoRebalancerTask::GetTabletLeader() (see auto_rebalancer.cc), so that dance with STLDeleteElements is necessary there as well. From the other side, maybe it's worth introducing an additional constructor for TSInfosDict or an additional class for TSInfosDict to be used outside of the CatalogManager. That class would store the heap-allocated elements of TSInfoPB wrapped into std::unique_ptr or call STLDeleteElements in the destructor. TSInfosDict's constructor should be made private, and CatalogManager and Master should be friended to use it. -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 07 Jul 2020 23:13:32 + Gerrit-HasComments: No
[kudu-CR] Optimize GetTableLocations for better arena use
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16137 ) Change subject: Optimize GetTableLocations for better arena use .. Patch Set 7: Code-Review+1 (2 comments) Looks good to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/master_service.cc@408 PS7, Line 408: AddAllocated If keeping the CHECK() above, does it make sense to switch to UnsafeArenaAddAllocated() here? Alternatively, maybe switch CHECK_EQ() into DCHECK_EQ() since AddAllocated() performs similar validation on itself and do copying if the arenas are different? http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/16137/7/src/kudu/master/ts_descriptor.cc@220 PS7, Line 220: CHECK nit: I guess if registration is an empty std::unique_ptr wrapper, the process will get a SIGSEGV at line 221. With that, do you think having DCHECK() is good enough here? -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 07 Jul 2020 23:05:11 + Gerrit-HasComments: Yes
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#7). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 6 files changed, 48 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/7 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#4). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 5 files changed, 43 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/4 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16137 to look at the new patch set (#3). Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 5 files changed, 43 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/3 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Optimize GetTableLocations for better arena use
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/16137 to review the following change. Change subject: Optimize GetTableLocations for better arena use .. Optimize GetTableLocations for better arena use Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea --- M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 5 files changed, 42 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16137/1 -- To view, visit http://gerrit.cloudera.org:8080/16137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I65aa6b5a9854a4659d1e350e1461ad63c03d5fea Gerrit-Change-Number: 16137 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin
