Todd Lipcon 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 se yea, I guess so, but that doesn't really make sense, because as the client reads data it doesn't actually "consume" any state, so you don't need to do anything special to restart. And it means that if the Init() takes longer than the timeout, you'll never make progress. 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 th Done 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 > Given the amount of IO going on here (opening n blocks, reading a superbloc good point, both of you. We don't need any locks for Init() at all, since we now don't expose the session until after it successfully initted. We certainly don't need the try_to_lock nonsense I wrote above :) It was left-over from an earlier revision of the patch where I was still trying to support 're-init'. Nevertheless, I agree that a mutex is more appropriate here and for the sessions_lock_ in remote_bootstrap_service.cc, so changed those. Line 75: if (!l.owns_lock()) { > This check seems somewhat unnecessary. The "fail on second Init() call" mak yea that was junk, replaced with a CHECK Line 76: return Status::IllegalState("Remote bootstrap session already in used"); > Nit: you mean "already in use"? Or "already used"? removed all this -- 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: Todd Lipcon <[email protected]> Gerrit-Reviewer: song bruce zhang <[email protected]> Gerrit-HasComments: Yes
