Dan Burkert has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 7:


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.

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).

File src/kudu/client/client.h:

PS6, Line 1029: be

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). 

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.

File src/kudu/util/monotime.h:

Line 133:   friend MonoTime operator -(const MonoTime& t, const MonoDelta& 
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 <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to