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.
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.
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,
> 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
Line 398: data->NextBatcher(session, 0, nullptr);
> I find the semantics of -1, 0, and >0 to be non-obvious. Perhaps we can con
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 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>