Alexey Serbin has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 6: (7 comments) 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. 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'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? http://gerrit.cloudera.org:8080/#/c/3952/6/src/kudu/client/client.h File src/kudu/client/client.h: PS6, Line 1029: be > s/be/become Done 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, > 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: https://en.wikipedia.org/wiki/Watermark_(disambiguation) 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. 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); > 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-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
