Dan Burkert has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 6: (5 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>(); > OK, that sounds reasonable. Do I understand correctly that you suggest to I'm suggesting that maybe it's not needed, because the operation is infallible. It looks like you added Status returns instead of void in a bunch of places, but I couldn't find any places which could actually fail. 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, > Why not? If we exposed them in the documentation, a user could link a bina We don't want to impose a configuration framework on clients. Imagine if the application used multiple libraries, all of which came with their own configuration framework. This is a real issue in Hadoop where the HDFS client libraries ship with a configuration framework, and it's an absolute disaster. PS6, Line 91: waterline > That's fun, because originally I had put 'watermark' and then figured that Yah, I just suggest watermark to keep it consistent with the Java impl. In reality the words are synonymous. Line 98: messenger->ScheduleOnReactor( > Yes, the Java implementation is different -- it does not have batchers and I'm not quite following. It sounds like the C++ client does not have an equivalent to the PleaseThrottle exception, and instead we block when there are no buffers available. That makes sense. But why in that circumstance is there a watermark? Why would you want to block earlier than necessary? 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); > I added them here because the tests I added rely on those. Thanks! -- 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
