Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10997 )
Change subject: WIP: KUDU-2509 fix use-after-free in case of WAL replay error ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/10997/2//COMMIT_MSG Commit Message: PS2: > Could you prepare a second version of this patch that's much more targeted, That's a very good idea. Done. http://gerrit.cloudera.org:8080/#/c/10997/2/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/10997/2/src/kudu/tablet/tablet_bootstrap.cc@629 PS2, Line 629: tablet_.swap(tablet); > Nit: I tend to prefer this style: I think using std::move() instead of swap() is a better choice when the target object is not yet constructed. Maybe, sometimes it's important to destroy the lhs prior to the assignment, so swap() would not be an option. And if we want to invalidate the rhs, std::move() is also a better choice. >From the other side, sometimes the move assignment operator is implemented >using swap() (and that seems to be true for scoped_refptr()). However, that's >not always the case, and probably in such cases using std::move() is better. I also found this piece: https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments OK, I'll replace it with std::move() in the dedicated changelist (this one will be re-targeted for back-porting into other branches). http://gerrit.cloudera.org:8080/#/c/10997/2/src/kudu/tablet/tablet_bootstrap.cc@891 PS2, Line 891: entry_ptr->release(); > Can you change OpIndexToEntryMap to store unique_ptr<LogEntryPB> for the va I looked at that briefly, and that seems doable, yes. I just didn't want to broaden this changelist too much, however now when we have a localized fix targeted for backporting, I think I'll do that as well. -- To view, visit http://gerrit.cloudera.org:8080/10997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11373b1cc34d9e2e0181bee2d3841b49022218ed Gerrit-Change-Number: 10997 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 20 Jul 2018 05:09:22 +0000 Gerrit-HasComments: Yes
