Adar Dembo has posted comments on this change. Change subject: KUDU-1436. Concurrent remote bootstrap session starts can crash or corrupt tablets ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2913/1/src/kudu/tserver/remote_bootstrap_service.cc File src/kudu/tserver/remote_bootstrap_service.cc: Line 139 Hmm, why do you think this "reinit" path existed? Was it to "rewind" the session state, so that the RB client can start over from the beginning? Line 136: LOG(INFO) << "Re-sending initialization info for existing remote bootstrap session" Nit: if you're already reworking this, mind using strings::Substitute so that it's easier to visualize the entire sentence? http://gerrit.cloudera.org:8080/#/c/2913/1/src/kudu/tserver/remote_bootstrap_session.cc File src/kudu/tserver/remote_bootstrap_session.cc: Line 74: std > As have already removed calling "session->init" in else of BeginRemoteBoot Given the amount of IO going on here (opening n blocks, reading a superblock from disk, etc.), a sleeping lock would be more appropriate. Though with your change, I suppose we'll never actually spin waiting for the lock, at least not in Init(). Line 75: if (!l.owns_lock()) { This check seems somewhat unnecessary. The "fail on second Init() call" makes sense, but you could achieve that with the old code + a check on initted_. Why bother with the try lock? Do you think it's important for an errant second Init() call to avoid blocking on the first call to complete? Line 76: return Status::IllegalState("Remote bootstrap session already in used"); Nit: you mean "already in use"? Or "already used"? -- To view, visit http://gerrit.cloudera.org:8080/2913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ec55e7b51da9e7c795c18146f04d539d1e44ccd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: song bruce zhang <[email protected]> Gerrit-HasComments: Yes
