Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14061 )
Change subject: [tablet] Fixed the bug of DeltaTracker::CountDeletedRows ...................................................................... Patch Set 4: (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: workload.set_write_pattern(TestWorkload::INSERT_RANDOM_ROWS_WITH_DELETE); : workload.set_write_batch_size(3); Why are these necessary? Please doc. http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/raft_consensus-itest.cc@3139 PS4, Line 3139: Nit: did you mean to add this empty line at the end? 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: // Insert random row keys and delete them. Nit: "Insert random rows, then delete them." 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: auto check_error_func = [](KuduSession* session, TestWorkload* workload) { Since you have to pass 'this', how about you make this a named member function instead? http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc@168 PS4, Line 168: KuduPartialRow *row Nit: the previous style ('KuduPartialRow*' ) is more correct. http://gerrit.cloudera.org:8080/#/c/14061/4/src/kudu/integration-tests/test_workload.cc@237 PS4, Line 237: int64_t expected_row_count = : write_pattern_ == INSERT_RANDOM_ROWS_WITH_DELETE ? 0 : rows_inserted_.Load(); Should note that when INSERT_RANDOM_ROWS_WITH_DELETE is used, ReadThread doesn't really verify anything except that a scan works. http://gerrit.cloudera.org:8080/#/c/14061/2/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/14061/2/src/kudu/tablet/delta_tracker.cc@746 PS2, Line 746: > Yeah, very good case!! L746 updates the in-memory tablet superblock to reflect the DMS flush. L747 atomically writes that new tablet superblock to disk. If the tserver crashes in between, the changes made by L746 are lost. But that's OK; the post-crash state is correct as long as it reflects either the entirety of the DMS flush, or none of it. 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: // Lock the component_lock_ in exclusive mode. : std::lock_guard<rw_spinlock> lock(component_lock_); : // Merge the deleted row count of the old DMS to the RowSetMetadata : // and reset deleted_row_count_. These comments just duplicate the actual code. It would be more interesting to instead describe _why_ this is necessary. i.e. why do we need to take component_lock_ in exclusive mode before doing this? 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: Status CommitRedoDeltaDataBlock(int64_t dms_id, Could you doc this function? -- 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: 4 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: Mon, 19 Aug 2019 19:19:38 +0000 Gerrit-HasComments: Yes
