Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets ...................................................................... Patch Set 33: (9 comments) http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h File src/kudu/tablet/rowset_metadata.h: http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h@85 PS32, Line 85: Status Flush() const; nit: it's pretty weird to mark this const, since it's technically mutating *something*, even if it goes through a const member to do that. Was this needed for some reason? http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@371 PS32, Line 371: col_ids nit: just for future reference, doing random renames in a big patch is a bit distracting, unclear how this is particularly better, anyway... http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@668 PS32, Line 668: mutable simple_spinlock lock_; nits: 1) Perhaps rename this state_lock_ since lock_ is so generic it sounds like the only lock in the class when that is far from the truth (there will now be 4) 2) Also, specify the lock acquisition order when / if this lock is acquired simultaneous to other locks. 3) if you have to take this lock_ to access state_ then it's better to declare it before state_ so that acquiring this lock_ probably puts state_ into the L1 cache automatically. http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h@457 PS31, Line 457: state_ = s; it would be nice to assert on the state transitions in this function to enforce the above DAG. http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@320 PS31, Line 320: CancelMaintenanceOps(); Why do we need to make this call in Stop() ? Can't the ops just cancel themselves if they notice that the tablet is no longer running? http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@1398 PS31, Line 1398: for (MaintenanceOp* op : maintenance_ops_) { If we don't want to hold the lock while unregistering the ops then we need to first copy the map before dropping the lock and iterating through the copy. Or, if this method could be called simultaneously by multiple threads, we likely have to be even more careful than that. http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@952 PS33, Line 952: s = InsertOrUpsertUnlocked(tx_state, row_op, stats); : if (s.IsAlreadyPresent()) { : return Status::OK(); : } : return s; : : case RowOperationsPB::UPDATE: : case RowOperationsPB::DELETE: : s = MutateRowUnlocked(tx_state, row_op, stats); : if (s.IsNotFound()) { : return Status::OK(); : } : return s; these look like good changes... but are they at all related to the theme of the patch? Do you think we need additional test coverage or something? http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc File src/kudu/tablet/transactions/transaction_driver.cc: http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc@518 PS31, Line 518: CHECK(tablet->HasBeenStopped()); Doesn't HandleFailure already do this? As long as we document that it takes care of this then we can delegate that responsibility to that function. http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc@1764 PS31, Line 1764: fall typo: fail -- To view, visit http://gerrit.cloudera.org:8080/7439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42 Gerrit-Change-Number: 7439 Gerrit-PatchSet: 33 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2017 04:11:10 +0000 Gerrit-HasComments: Yes
