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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
