Adar Dembo has posted comments on this change.
Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
Patch Set 15:
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.
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,
> 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?
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 http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>