Adar Dembo 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: (4 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, one we could more easily backport to older releases? http://gerrit.cloudera.org:8080/#/c/10997/2/src/kudu/consensus/log_util.cc File src/kudu/consensus/log_util.cc: http://gerrit.cloudera.org:8080/#/c/10997/2/src/kudu/consensus/log_util.cc@186 PS2, Line 186: pending_entries_.front().swap(*entry); Nit: *entry = std::move(pending_entries_.front()) 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: tablet_ = std::move(tablet); Thoughts? 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 value type? Seems like that would simplify some of this too, though maybe it's not possible (i.e. LogEntryPB is actually shared ownership), or maybe it's too big of a change. -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 20 Jul 2018 02:52:41 +0000 Gerrit-HasComments: Yes
