Dan Burkert has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 23:


Nice.  This is much more understandable than previously.

File src/kudu/client/batcher.cc:

Line 474:     Status s = had_errors_ ? Status::Incomplete("Some errors 
I don't think Incomplete is the right status here; the flush is complete.

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

File src/kudu/client/client.cc:

Line 322:                                            hp);
this can go on the line above.

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

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_.

File src/kudu/client/session-internal.h:

PS23, Line 46: Aply

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

Reply via email to