Dan Burkert has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 23: (16 comments) Nice. This is much more understandable than previously. http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 474: Status s = had_errors_ ? Status::Incomplete("Some errors occurred") I don't think Incomplete is the right status here; the flush is complete. http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS23, Line 2360: there the current batcher is at place Not sure what this is trying to say, maybe: there is a current batch http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 322: hp); this can go on the line above. http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: Line 63: TimeBasedFlushInit(session); How about checking if we are in AUTO_FLUSH_BACKGROUND first? I know this is basically a no-op if not, but it does schedule a background task. Line 118: // Thread-safety note: the buffer_bytes_limit_ is not supposed to be accessed These thread safety notes (here and below) would be better in the header declaration doc. Line 156: Status KuduSession::Data::SetMaxBatchersNum(unsigned int max_num) { How come this one doesn't check HasPendingOperations? PS23, Line 283: nil NULL PS23, Line 356: } : if ( should this be an else if? Line 368: buffer_bytes_used_ += required_size; hmm, this seems a little early. I can't think of a reason it would matter, but it seems to me this counter shouldn't be updated until the batcher is available and we are just about to or just have added the op to the batcher. Otherwise we are updating the counter, potentially dropping the lock via the cond_.Wait() call, and then later actually adding the op. Line 379: if (!batcher_) { Will the batcher_ ever be non-null here? I can't think of a reason why it would be. Maybe a DCHECK is in order instead of the conditional. Line 399: buffer_bytes_used_ -= required_size; Another good reason not to increment this counter until after the op is successfully added to the batcher_. http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/session-internal.h File src/kudu/client/session-internal.h: PS23, Line 46: Aply Apply PS23, Line 91: Return once no more pending operations are left. this sentence seems redundant. PS23, Line 91: finished their flushing finish flushing Line 191: std::unordered_set<internal::Batcher*> flushed_batchers_; Why is it necessary to hold on to these batchers? I see that it's used for calling HasPendingOperations, but wouldn't it be enough to check that batches_num_ > 0 in that case? PS23, Line 199: // protected by batch_ctl_mutex_ the setter doesn't take batch_ctl_mutex_ -- To view, visit http://gerrit.cloudera.org:8080/3952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8 Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
