Todd Lipcon has posted comments on this change. Change subject: bootstrap: filter row operations before decoding writes ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/3037/1/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 1388 > Didn't find this logging useful enough to retain? I don't have an opinion, yea, it was pretty darn noisy for VLOG(1) and we don't really have anything printable anymore at this point Line 301: struct Stats { > The use of "ops" here to mean entire messages is a bit confusing since each yea, agreed, but at least the comments down below make it clear. Line 1170: RETURN_NOT_OK(FilterOperation(orig_op_result, &f)); > There's no fancy way to pass the address of flushed_by_op[i] into FilterOpe nope, normally one could, except that vector<bool> is magical and is actually a bitset underneath. So, the actual elements don't have addresses and we have to use a local here in between. todd@todd-ThinkPad-T540p:/tmp$ cat test.cc #include <iostream> #include <vector> using namespace std; void f() { vector<bool> b(10); cout << &b[3]; } todd@todd-ThinkPad-T540p:/tmp$ g++ -o test test.cc test.cc: In function ‘void f()’: test.cc:7:15: error: taking address of temporary [-fpermissive] cout << &b[3]; ^ Line 1222: // Append the commit msg to the log but replace the result with the new one. > Nit: indentation. Also, the comment may no longer really apply here. Done Line 1300: CHECK_EQ(tx_state->row_ops().size(), already_flushed.size()); > I think this is reasonable as a DCHECK, no? Done Line 1398: if (PREDICT_FALSE(num_mutated_stores == 0 || num_mutated_stores > 2)) { > The new code relaxes the requirement that inserts have just one mutated sto yea, I noted that as well. It's true, but that was always just a sort of assertion and not really critical as part of the normal correct flow - we have lots of other tests which would fail if we started doing this bit incorrectly. Line 1417: *already_flushed = false; > In general, I don't think we should write to an OUT parameter if we return Done -- To view, visit http://gerrit.cloudera.org:8080/3037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida7c54b122d8abee407fb8863a911a4d3887a9cb 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: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
