Adar Dembo has posted comments on this change. Change subject: log: refactor log reading into an iterator-like interface ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/3038/2/src/kudu/consensus/log_util.cc File src/kudu/consensus/log_util.cc: Line 187: return s.CloneAndPrepend(Substitute("Error reading from log $0", seg_->path_)); Nit: lower-case for Error? This will probably be appended to another Status up the stack, right? Though I see a number of other places here use upper-case for the beginning of their error messages, so maybe it's more consistent with the rest of the file this way. Line 223: // TODO: can remove some params here? What is this referring to? There's just the status param. Line 231: for (const auto& r : recent_entries_) { It's a fair amount of work to maintain recent_entries_ and I see it's just for MakeCorruptionStatus(). Is it worth it? http://gerrit.cloudera.org:8080/#/c/3038/2/src/kudu/consensus/log_util.h File src/kudu/consensus/log_util.h: Line 130: int64_t batches_read_; Nit: num_batches_read_ Line 201: std::unique_ptr<LogEntryReader> NewLogEntryReader(); Curious why you picked this approach to constructing a LogEntryReader instead of something like: class LogEntryReader { public: LogEntryReader(ReadableLogSegment* log_segment); ... } The above lets you stack allocate LogEntryReaders, which would at least be useful in your port of ReadEntries(). Maybe it's less useful in the follow-on work. -- To view, visit http://gerrit.cloudera.org:8080/3038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5727951664600b7a591a6c892ea1e62469ceb109 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
