Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/14061 )
Change subject: [tablet] Fixed the bug of DeltaTracker::CountDeletedRows ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/raft_consensus-itest.cc@969 PS4, Line 969: // Otherwise, test deleting data on MRS. : int32_t write_interval_millis = 0 > Why are these necessary? Please doc. Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/raft_consensus-itest.cc@3139 PS4, Line 3139: WriteRequestPB w_req; > Nit: did you mean to add this empty line at the end? a slip of a pen http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.h File src/kudu/integration-tests/test_workload.h: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.h@152 PS4, Line 152: enum WritePattern { > Nit: "Insert random rows, then delete them." Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc File src/kudu/integration-tests/test_workload.cc: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc@140 PS4, Line 140: while (should_run_.Load()) { > Since you have to pass 'this', how about you make this a named member funct Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc@168 PS4, Line 168: // Note: overridi > Nit: the previous style ('KuduPartialRow*' ) is more correct. Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc@237 PS4, Line 237: : size_t TestWorkload::GetNumberOfErrors(kudu::client::KuduSession* session) { > Should note that when INSERT_RANDOM_ROWS_WITH_DELETE is used, ReadThread do Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/delta_tracker.cc@748 PS4, Line 748: // Merge the deleted row count of the old DMS to the RowSetMetadata : // and reset deleted_row_count_ should be atomic, so we lock the : // component_lock_ in exclusive mode. : std::lock_guard<rw_spinlock> loc > These comments just duplicate the actual code. It would be more interesting Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/rowset_metadata.h File src/kudu/tablet/rowset_metadata.h: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/rowset_metadata.h@120 PS4, Line 120: // Atomically commit the new redo delta block to RowSetMetadata. > Could you doc this function? Done http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/rowset_metadata.cc File src/kudu/tablet/rowset_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/tablet/rowset_metadata.cc@291 PS4, Line 291: std::lock_guard<LockType> l(lock_); > Additional locks to old tables that do not support live row > counting. Emm, I think our newly created table will turn this on by default, so this should be acceptable. -- To view, visit http://gerrit.cloudera.org:8080/14061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9bb4456123087778c9dc799777c5990938a84fdf Gerrit-Change-Number: 14061 Gerrit-PatchSet: 9 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 20 Aug 2019 11:11:36 +0000 Gerrit-HasComments: Yes
