Adar Dembo has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 15:

File src/kudu/client/client.h:

Line 1146:   Status SetMutationBufferFlushWatermark(double watermark_pct)
> Will do.  BTW, do we need getters for those parameters?  I think getters mi
I don't think it's a big deal either way. Sessions are cheap (especially now 
that they don't have a dedicated thread to flush in the background) and so I 
tend to think they're short-lived, which means it's easy for the application to 
remember what value was provided here.
File src/kudu/client/

PS15, Line 64: -1,
> This is to force getting a new batcher and flushing the prior, if any, even
I don't think the existing behavior is necessarily worth preserving; an empty 
flush doesn't actually accomplish anything.

In any case, that wasn't what my comment was about; I was asking about the 
semantic difference between '-1' and '0' as the watermark value.

Line 66:     PeriodicFlushTask(Status::OK(), messenger_, session);
> OK, I'll change it then.
Sorry, I don't understand what you're suggesting. Can you explain it in more 

Line 109:   flow_control_cond_.Broadcast();
> The original idea was to get rid of those limitations like empty buffer whe
Nope, simplifying the implementation is the underlying motivation. But I think 
it's enough; the clients are generally complicated code and aren't as well 
tested as the rest of Kudu.

Line 135:   CHECK_GE(timeout_ms, 0);
> Yep.  What we can do instead is to set it to 0 if the specified parameter w
I think that's reasonable provided it's documented.

PS15, Line 230:   RETURN_NOT_OK(TryApplyWriteOp(write_op, &required_size,
              :                                 &current_flush_mode, 
> Yep, having those facts simplifies this.  Again, I was under impression the
I don't understand why friendship factors into this: both ApplyWriteOp and 
TryApplyWriteOp are members of KuduSession::Data, so they should be equivalent 
w.r.t. calling Batcher::SizeInBuffer().

Line 398:   data->NextBatcher(session, 0, nullptr);
> Nope.  It would, however, if the second parameter were -1 (as you noticed t
I find the semantics of -1, 0, and >0 to be non-obvious. Perhaps we can convey 
the desired behavior from caller to PeriodicFlushTask() in another way? Through 
an additional parameter, an enum, or something like that?
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.
> This is because the flush_mode_ can change in the middle and the code which
I don't understand how flush_mode_ can change in the middle of ApplyWriteOp if 
the session may only be used by a single thread. Or is this a holdover from 
before, when it was OK for multiple threads to write to a session?

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