Dan Burkert has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 7: (7 comments) Incremental review, since I see a new revision has been put up. I don't think this is going to make 0.10, so I'm going to preempt it in the meantime. 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 fail. I think as it's written will be quite problematic, I can see this being a common source of nullptr dereferences (if in fact there are cases where it can fail). 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 http://gerrit.cloudera.org:8080/#/c/3952/6/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: Line 30: I think gflags don't apply to the client, so these will not really be configurable. I think setting the default to be a constant and exposing mutators on the Session instance is the best we can do. Line 59: formatting (no wrap on initial argument, and 4 spaces for initializer list). https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists. A lot of the methods below are wrapped too much as well. PS6, Line 91: We typically use 'watermark' instead of 'waterline' (at least in the Java impl). Line 98: const bool over_waterline = This looks like it's flushing the current buffer if it's over the waterline, but that's not exactly what the Java client does. If a write comes in over the waterline, a PleaseThrottle exception is passed back to the user. Also, this only ever happens if there isn't a free buffer available. 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. -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
