Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 23:


Dan & Adar, thank you for the review.  I'll post updated version as soon as I 
make sure the tests are passing OK.
File src/kudu/client/

Line 474:     Status s = had_errors_ ? Status::Incomplete("Some errors 
> I don't think Incomplete is the right status here; the flush is complete.
OK, that reasoning makes sense.

However, I thought IOError might be irrelevant here as well.  Or this is the 
best match among all possible status codes?  There is seemingly more generic 
RunTimeError, but I'm not sure that's the right case to use it.

I'll try to return IOError back to get back to the original, and meanwhile we 
can discuss it separately.
File src/kudu/client/

PS23, Line 2360: there the current batcher is at place
> Not sure what this is trying to say, maybe:
ops, my bad.

Should have been something like 'OK, now the current batcher should be at 

Will fix.
File src/kudu/client/

Line 322:                                            hp);
> this can go on the line above.
File src/kudu/client/

Line 63:   TimeBasedFlushInit(session);
> How about checking if we are in AUTO_FLUSH_BACKGROUND first?  I know this i
The check you mentioned is done by TimeBasedFlushTask() method _before_ 
scheduling a task (i.e. no task scheduled if current mode is not 
AUTO_FLUSH_BACKGROUND).  So, I see no need to perform that check here.

I put all those ugly guts (synchronization, etc.) into that method with 
intention to avoid doing those checks elsewhere.

Line 63:   TimeBasedFlushInit(session);
> The funny thing is that, by definition, we cannot be in AUTO_FLUSH_BACKGROU
TimeBasedFlushInit() is no-op in other than AUTO_FLUSH_BACKGROUND mode.

Yes, it might be a bad name for the method.

What name would you recommend instead?

PS23, Line 105:    std::lock_guard<simple_spinlock> l(lock_);
> Since this isn't acquired on any hot path, what do you think of removing it
I thought it was better to keep those locks separate to avoid having one giant 
lock for all the methods.  Those locks based on simple_spinlock look lighter 
than based on mutex.  But as you mentioned, since we already have that for the 
most critical paths, and the rest with simple_spinlock are just for methods 
which almost never called, I think it's a good idea.

Will do, thanks!

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 de
I tried that, but from the reading perspective it looked worse -- there is not 
much of context when looking at the signature of a function/method.  However, 
in the body of a method, there is much more relevant context since you can see 
the logic of the method, more comments, corresponding variables/members, etc.

That's said, I'm hesitant to move those in the header.

However, if you feel strong about that and have a compelling reasoning which 
addresses my concern regarding the lack of context, I'll move those.

Line 156: Status KuduSession::Data::SetMaxBatchersNum(unsigned int max_num) {
> How come this one doesn't check HasPendingOperations?
I didn't see a restriction that would arise from the current design.

Actually, we can remove that for SetBufferFlushInterval() and other except for 
the KuduSession::SetFlushMode().

But as I see from our recent HipChat discussion, we want to keep us more space 
for the future, because once published, it's easier to make those restrictions 
less strict.  But making these more strict could be almost impossible in the 

So, I added the artificial restriction here as well.

Line 181:   FlushCurrentBatcher(1, cb);
> Nit: add a comment explaining the significance of 1 (i.e. why not 0?)

PS23, Line 188: nullptr
> I spent a lot of time wondering if the loss of the synchronizer and RETURN_
I did a research on that prior to using nullptr as a callback in the new code 
and I didn't see any cases where an error would be dropped on the floor (i.e. 
in all cases where Batcher::had_errors_ is set to true, the error is registered 
into the Batcher::error_collector_).  So, it's safe as far as I can see.  But 
I'll add corresponding comments into the batcher and for the 
FlushCurrentBatcher() methods to clarify on that.

I think it's a good idea to document the difference.  I even thought I had done 
it, but re-visiting the client.h comments revealed that I missed that part.  
Thank you for the reminder -- I'll add corresponding comments there.

PS23, Line 283: nil

PS23, Line 296: The flush_mode_ is accessed
              :   // from the background time-based flush task, but it accesses 
              :   // only for reading.
> TSAN may complain about this. ISTR it doesn't like accesses to the same mem
OK, I see.  I suspected something like that, but since I haven't seen anything 
from TSAN, I though it was happy.

I will protect flush_mode_ as needed.  Thanks!

Line 354:         batch_ctl_cond_.Wait();
> This isn't going to work for ApplyAsync(). Now, we haven't actually impleme
Yep.  For ApplyAsync() the control would go into the next clause instead.  So, 
it's about having something like

if (apply_mode == SYNC && flush_mode == AUTO_FLUSH_BACKGROUND) {
} else if (...) {

For the batchers limit it will be the something like

if (apply_mode == ASYNC && !batcher_ && batchers_num_limit_ != 0 && 
batchers_num_ >= bathers_num_limit_) {
  return Status::Aborted("too many batchers");

Basically, we are introducing an additional parameter for this method: 
apply_mode.  May be, brushing up the code a little.

Then it should work as expected, right?

PS23, Line 356: }
              :     if (
> should this be an else if?
Yes, indeed.  Good observation!  Somehow I missed this while doing the 

Line 368:     buffer_bytes_used_ += required_size;
> hmm, this seems a little early.  I can't think of a reason it would matter,
Good catch!

It seems this is a remnant from previous multi-mutex version, where there were 
separate mutexes for batcher and byte counter.

Line 379:       if (!batcher_) {
> Will the batcher_ ever be non-null here?  I can't think of a reason why it 
Another multi-thread remnant -- that were needed when it was possible to have 
multiple threads calling Apply().

Having DCHECK() would not hurt, right.

Line 399:       buffer_bytes_used_ -= required_size;
> Another good reason not to increment this counter until after the op is suc
File src/kudu/client/session-internal.h:

PS23, Line 42: It's
> Nit: Its

PS23, Line 43: implemented
> Nit: don't need this word.

PS23, Line 46: Aply
> Apply

PS23, Line 91: finished their flushing
> finish flushing

PS23, Line 91: Return once no more pending operations are left.
> this sentence seems redundant.

PS23, Line 128:   // The self-rescheduling task to flush write operations which 
have been
              :   // accumulating for too long (controlled by flush_interval_).
              :   // This does real work only in case of AUTO_FLUSH_BACKGROUND 
> Should document what 'do_startup_check' means.

PS23, Line 172: requries
> Nit: "requires"

PS23, Line 172: which
> Nit: "whose"

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
That's a good question!  Besides using those for current implementation of 
HasPendingOperations() methods, those are also used for independent count of 
bytes of currently pending operations: GetPendingOperationsSize().  I see this 
method is currently used only in tests, so it might be a good idea to resent to 
direct call of the KuduSession::Data internals (as those tests now do anyways), 
and try to get rid of this set of pointers.

May be, it was also good from the debugging perspective?

PS23, Line 199: // protected by batch_ctl_mutex_
> the setter doesn't take batch_ctl_mutex_
Thanks -- it seems this comment is stale and is left from times when 
KuduSession was thread-safe.  Will fix.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 23
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to