Alexey Serbin has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3952/6/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 400:       return shared_ptr<KuduSession>();
> I'm suggesting that maybe it's not needed, because the operation is infalli
That's true -- the method returned Status since I brought it from the previous 
revision where I used a separate flusher thread, and Start() could return an 
error if it could not start the flusher thread.

Thank you for pointing at that -- I removed the Status return code, so now it 
returns void.


http://gerrit.cloudera.org:8080/#/c/3952/6/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

Line 30: DEFINE_int32(client_buffer_bytes_limit, 7 * 1024 * 1024,
> We don't want to impose a configuration framework on clients.  Imagine if t
Yes, this reasoning makes sense to me.  Since David suggested to use glags for 
those parameters, I though that was a good idea.  Probably, David did not mean 
to expose those to clients.  But then the only usage for them is throughout the 
tests.  OK, that sounds good to me.  If there is a necessity to expose those to 
client applications, we can add that later.


Line 98:       messenger->ScheduleOnReactor(
> I'm not quite following.  It sounds like the C++ client does not have an eq
This code is about determine whether to perform flush.  This is not about 
determine whether to block (that when-to-block code you could find in 
KuduSession::Data::ApplyWriteOp()).

The waterline/watermark criterion is to determine when it's necessary to flush 
the current batcher.  The idea is too check how much of the buffer is taken by 
the freshly submitted (not yet scheduled for flush) operations: if it's more 
than the specified percentage, then flush the current batcher.  Only the 
current batcher has freshly submitted (not yet scheduled for flush) operations.

In my previous comment, I tried to point to the presence of batchers in the C++ 
client code. Probably, the confusion is due to the fact that Java 
implementation does not have those.  In C++ client, there might be several 
batchers accommodating pending operations.  However, the limit on the total 
size of pending operations is enforced -- in total, all currently active 
batchers cannot consume more space for their pending operations than the limit 
specified via the KuduSession::SetMutationBufferSpace() method.

Does it help to resolve the confusion (if any)?


http://gerrit.cloudera.org:8080/#/c/3952/6/src/kudu/util/monotime.h
File src/kudu/util/monotime.h:

Line 133:   friend MonoTime operator -(const MonoTime& t, const MonoDelta& 
delta);
> Thanks!
Actually, thank you for pointing to this.  For some reason I overlooked the 
fact that having small independent patches would benefit everyone.


-- 
To view, visit http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
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
Gerrit-HasComments: Yes

Reply via email to