Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 15:


Thank you for your feedback.  I'm posting a new version in a minute.
File src/kudu/client/client.h:

Line 1146:   Status SetMutationBufferFlushWatermark(double watermark_pct)
> I don't think it's a big deal either way. Sessions are cheap (especially no
OK, that seems to be good reasoning to me.  No getters then.
File src/kudu/client/

PS15, Line 64: -1,
> I don't think the existing behavior is necessarily worth preserving; an emp
All right, I removed the semantics of flushing an empty batcher.  That will 
make a difference when there is a tight limit on maximum number of batchers 
allowed to exist simultaneously.

Line 66:     PeriodicFlushTask(Status::OK(), messenger_, session);
> Sorry, I don't understand what you're suggesting. Can you explain it in mor
I'm sorry for that poorly articulated and poorly written piece.

To clarify on that thought, I'm copy-pasting a comment from the code which 
implements that idea, please see below.


Let's measure age of a batcher as the time elapsed from the moment of adding 
the very first operation into the batcher.
The idea is to flush a batcher when its age is very close
to the flush_interval_: let's call it 'batcher flush age'.
If current batcher hasn't reached its flush age yet,
just re-schedule the task to re-evaluate the age of
then-will-be-current batcher, so if the current batcher
is still current at that time, it will be exactly
of its flush age.


Doing so allows to address the issue you pointed at.

Line 109:   flow_control_cond_.Broadcast();
> Nope, simplifying the implementation is the underlying motivation. But I th
OK, I see.  It seems that after recent discussions an additional reason 
appeared: we want to have some sort of parity with the Java client.

Line 135:   CHECK_GE(timeout_ms, 0);
> I think that's reasonable provided it's documented.

PS15, Line 230:   RETURN_NOT_OK(TryApplyWriteOp(write_op, &required_size,
              :                                 &current_flush_mode, 
> I don't understand why friendship factors into this: both ApplyWriteOp and 
I thought you were talking about calling the batcher directly.  Yes, I 
addressed the required_size so it's no longer an output parameter of the 
TryApplyWriteOp() method.

Line 398:   data->NextBatcher(session, 0, nullptr);
> I find the semantics of -1, 0, and >0 to be non-obvious. Perhaps we can con
File src/kudu/client/session-internal.h:

PS15, Line 87: On successful return, the output flush_mode parameter is set
             :   // to the effective flush mode.
> I don't understand how flush_mode_ can change in the middle of ApplyWriteOp
Yes, this is a holdover.  The comment was written when I was under impression 
we allow to have multiple threads working with a single KuduSession object.  
Also, calls of Apply() from different threads are not relevant here -- 
actually, the reason was that other thread might call 
KuduSession::SetFlushMode() while waiting on the condition variable.

But now all those concerns are gone.  We don't allow to change the mode while 
operations are pending and we don't allow making calls to a single KuduSession 
object from multiple threads.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 15
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