Dinesh Bhat has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 19:


Hi Alexey,

went through this patch more from knowledge point of view, so feel free to 
ignore any review comments you find not useful. Also I haven't completely 
digested all the routines in sesion-internal.cc, so I may as well take a second 
pass tomorrow later if time permits.

File src/kudu/client/batcher.cc:

PS18, Line 522: we 
Does it make sense to add a coment that callers are expected to serialize 
during Add ?

File src/kudu/client/batcher.h:

Line 168: 
typo: the the

PS16, Line 169: ck

File src/kudu/client/batcher.h:

PS17, Line 122: int64_t
Naive doubt here: If Load() is returning a non-atomic value we seem to be 
accessing the atomic value of buffer_bytes_used_ in write path vs non-atomic in 
read path ?

File src/kudu/client/client.cc:

Line 692:   : data_(new KuduSession::Data(client, client->data_->messenger_)) {
I haven't looked at how data_ being set, just wondering if data_ could be NULL 
since it seems to be a regular ptr.

PS18, Line 736: int
uint ?

PS18, Line 740: int
uint ?

File src/kudu/client/client.h:

Line 1017:   /// Modes of flush operations.
Is there a default mode of op here ? if so, can we mention it in comment ?

Line 1038:     /// batch is sent and the reclamed space is available for new 
typo: reclamed

Line 1040:     /// @todo Provide an API for the user to specify a callback to 
do their own
If this is not specific to this mode alone, we could move this todo in the 
beginning perhaps ?

Line 1044:     ///   (probably the messenger IO threads?).
I guess this todo is already addressed ?

File src/kudu/client/session-internal.cc:

PS18, Line 448:       
Nice !! Liked these ascii diagrams.

File src/kudu/client/session-internal.cc:

PS19, Line 233: weak_session
any significance of usage of 'weak' session ? :)

PS19, Line 263: (
Isn't this redundant ? We could as well place NextBatcher above ?
do  {
} while (need_pre_flush)

PS19, Line 270: lock_
I thought whole routine is serialized, so why another spinlock here ?

File src/kudu/client/session-internal.h:

PS18, Line 126: to
s/to/for ?

PS18, Line 165: cond_mutex_
Trivial nit: I suggest to rename this to flow_control_cond_mutex_ which could 
be self explanatory(and remove al the comment associated). Just a suggestion, 
feel free to ignore this.

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: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to