Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs ...................................................................... Patch Set 22: (20 comments) Currently discussing an alternate (maybe new tablet state) to Stop() http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc File src/kudu/integration-tests/stop_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@108 PS21, Line 108: writes.Setup(); > you don't need to insert data to get it to be visible - just creating the t Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@202 PS21, Line 202: // Now that the leader has been stopped, wait longer and ensure that no : // more writes get through. : // Note: a new leader will not be elected because the tablet has only been : // stopped and not shut down, so the leader can still s > this is odd. Wouldn't you expect it to re-elect a new leader? Or is this be Yep, exactly. The tablet is alive (this test was targeted at testing out the new Stopped state). Will add a comment. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@276 PS21, Line 276: > nit: s/should/will/ Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@277 PS21, Line 277: Close(); > nit: Close() ? Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280 PS21, Line 280: bool i > yea, not necessary Ack http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280 PS21, Line 280: () const > nit: Perhaps is_open() would be more intuitive than is_closed()? This is be Oh interesting link, thanks! I'm not sure I completely agree in this specific context, but I can see RETURN_NOT_OK(CheckNotClosed()) being confusing since it introduces a double negative. I'll try to avoid those where possible. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280 PS21, Line 280: bool i > Is "inline" useful here? I thought methods defined in their header declarat Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@281 PS21, Line 281: oad(); > I actually looked at the assembly generated by the default memory order, an Will leave as is then. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@316 PS21, Line 316: CheckOpen() co > nit: CheckOpen() seems more intuitive to me per the above Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@382 PS21, Line 382: open_; > I kinda disagree on this one since we often have this "shutdown_" type memb Hrm, interesting. Limited sample size, but it seems like for the non-init paths, the trend is to go from false-->true (e.g. various shutdown_, marking read_only_ in the LBM). Anyway, I think I agree with Mike, at least for private APIs. It's a bit easier to follow RETURN_NOT_OK(CheckIsActive) vs RETURN_NOT_OK(CheckNotClosed). http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@98 PS21, Line 98: !is_open())) { > nit: is_closed() Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@280 PS21, Line 280: did > did not Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121 PS21, Line 121: Currently, the tablet will only be Stopped as the tablet is shutting down. : // In the future, it can be Stopped when it hits a non-recov > It's not clear when one vs. the other should be called. Can you clarify her Ah, yeah given the current placement it does act entirely as a lifecycle change. The idea from an error-handling POV is that Stop() can complete without holding any locks and without waiting, and as such, it can be called immediately in the presence of failures, while Shutdown() must wait for various operations to finish (it locks component_lock_). That said, Stop() is idempotent, so putting it in Shutdown() couldn't hurt. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121 PS21, Line 121: Currently, the tablet will only be Stopped as the tablet is shutting down. : // In the future, it can be Stopped when it hits a non-recov > yea, agree this can still be clarified better whether it's required or opti Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@175 PS21, Line 175: > nit: missing "if" Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@176 PS21, Line 176: this transaction. : // > this bit of the comment is confusing in this context. Ah, thanks for the guidance! Done. Same elsewhere. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@183 PS21, Line 183: // Apply a single row operation, which must already be prepared. : // The result is set back into row_op->result. : // > same Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@480 PS21, Line 480: : // For each of the operations in 'tx_state', check for the presence of their : // row keys in the RowSets in the current RowSetTree (as deter > same Done http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc@905 PS21, Line 905: if (present) { : row_op->present_in_rowset = rowset; : break; : } : } : row_op->checked_present = true; > instead of copy-pasting this comment block a few times, maybe move this to Done, moved this to Stop()'s declaration. http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc@293 PS21, Line 293: / Only mark the peer > We call Tablet::Shutdown() here. So why not just have Tablet::Shutdown() ca Fair point. As per my other comment, I'll do this. -- 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: 22 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: Wed, 01 Nov 2017 19:34:10 +0000 Gerrit-HasComments: Yes
