helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13456 )
Change subject: [tablet] Support accurate count of rows ...................................................................... Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG Commit Message: PS11: > Some important limitations to be aware of are that the counts will be inacc Yea, that's true. And thank you for reminding.^_^ http://gerrit.cloudera.org:8080/#/c/13456/11//COMMIT_MSG@15 PS11, Line 15: 1. > nit: could you add a space between the numbers and the sentences and align Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/master/sys_catalog.cc@255 PS11, Line 255: /*supports_live_row_count=*/ true, > Is it important to have this for the SysCatalog tablet? I suppose it's not Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/delta_tracker.h@370 PS11, Line 370: : // When the flush completes, this is merged into the RowSetMetadata a > nit: strange newline formatting? Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/metadata.proto@138 PS11, Line 138: // Whether the tablet supports live row counting. : // It's only supported for the newly created ones, not for the ancient ones. : // When false, the 'live_row_count' in every RowSetDataPB is incorrect and : // should be ignored. > nit: Could you reword this slightly? Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tablet/tablet.cc@1919 PS11, Line 1919: int64_t tmp = 0; : RETURN_NOT_OK(comps->memrowset->CountLiveRows(&tmp)); : *count += tmp; : for (const shared_ptr<RowSet>& rowset : comps->rowsets->all_rowsets()) { : RETURN_NOT_OK(rowset->CountLiveRows(&tmp)); : *count += tmp; : } > nit: This will update `count` even if this method fails, which can be surpr Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@147 PS11, Line 147: virtual void GenerateTestData() { : Random rand(SeedRandom()); : NO_FATALS(tablet_replica_->tablet_metadata()-> : set_supports_live_row_count_for_tests(rand.Next() % 2)); : NO_FATALS(TabletCopyTest::GenerateTestData()); : } > nit: Would be good to add a comment description. Done http://gerrit.cloudera.org:8080/#/c/13456/11/src/kudu/tserver/tablet_copy_client-test.cc@412 PS11, Line 412: TEST_F(TabletCopyClientTest, TestSupportsLiveRowCount) { : ASSERT_OK(StartCopy()); : ASSERT_EQ(tablet_replica_->tablet_metadata()->supports_live_row_count(), : meta_->supports_live_row_count()); : } > Do you know whether it would be easy to verify that row counts as well? I a No, seems impossible. The 'BootstrapTablet' doesn't help. Maybe L786~L806 in tool_action_perf.cc can help, which means we need to scan the live rows and then sum it. But, I don't think it's necessary -_- -- To view, visit http://gerrit.cloudera.org:8080/13456 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e6378e289bb85024c29e96c2b153fc417ed6412 Gerrit-Change-Number: 13456 Gerrit-PatchSet: 11 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Wed, 05 Jun 2019 06:07:09 +0000 Gerrit-HasComments: Yes
