Adar Dembo has posted comments on this change. Change subject: bootstrap: filter row operations before decoding writes ......................................................................
Patch Set 1: (7 comments) I'm not that familiar with this code, but here's what I got... 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, but I thought I'd ask about it. Line 301: struct Stats { The use of "ops" here to mean entire messages is a bit confusing since each message can have multiple "operations". Not your fault, though. 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 FilterOperation directly and avoid the intermediate copy into f? Probably doesn't matter for actual performance, but fancy is fun. 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. Line 1300: CHECK_EQ(tx_state->row_ops().size(), already_flushed.size()); I think this is reasonable as a DCHECK, no? 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 store. Is that OK? Line 1417: *already_flushed = false; In general, I don't think we should write to an OUT parameter if we return failure (L1425). Can we defer this to L1428? -- 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-HasComments: Yes
