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.
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?
PS6, Line 1029: be
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.
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 http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins