Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 6:


Thank you for the review!

I posted a new version (patchset 7) because the LINT build failed for patchset 
6, so I wanted to fix that.
File src/kudu/client/

Line 400:       return shared_ptr<KuduSession>();
> I've traced this through, and I can't find any place that this can actually
OK, that sounds reasonable.  Do I understand correctly that you suggest to 
remove this check at all?
File src/kudu/client/client.h:

PS6, Line 1029: be
> s/be/become
File src/kudu/client/

Line 30: DEFINE_int32(client_buffer_bytes_limit, 7 * 1024 * 1024,
> I think gflags don't apply to the client, so these will not really be confi
Why not?  If we exposed them in the documentation, a user could link a binary 
with the library having those flags exposed.

But I agree: making those accessible via the client API would be easier to use, 
otherwise those are left hidden.  However, I don't like idea of mutators for 
there -- I would better set them in the constructor and keep then unchanged.  
Probably, those might be properties of some sort of configuration object.

How do you like the following: I'll mark those as hidden flags, which we will 
use only for testing purposes.  Later on, we can expose controls for those up 
to the client API level.  Does this make sense?

Line 59: KuduSession::Data::BackgroundFlusher::BackgroundFlusher(
> formatting (no wrap on initial argument, and 4 spaces for initializer list)
OK, will fix.

PS6, Line 91: waterline
> We typically use 'watermark' instead of 'waterline' (at least in the Java i
That's fun, because originally I had put 'watermark' and then figured that that 
was not the best term for that:

OK, I'll replace it with watermark.

Line 98:       messenger->ScheduleOnReactor(
> This looks like it's flushing the current buffer if it's over the waterline
Yes, the Java implementation is different -- it does not have batchers and 
other stuff.  Also, here the waterline is about amount of freshly added 
operations, not total size of the pending operations.
File src/kudu/util/monotime.h:

Line 133:   friend MonoTime operator -(const MonoTime& t, const MonoDelta& 
> Maybe split the changes to MonoTime into its own review.
I added them here because the tests I added rely on those.

All right, having that as a separate change will be better from codereview's 
point.  Will separate into a stand-alone change.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to