Dinesh Bhat has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
......................................................................


Patch Set 19:

(17 comments)

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.

http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/batcher.cc
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 ?


http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/batcher.h
File src/kudu/client/batcher.h:

Line 168: 
typo: the the


PS16, Line 169: ck
s/is/are/


http://gerrit.cloudera.org:8080/#/c/3952/17/src/kudu/client/batcher.h
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 ?


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/client.cc
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 ?


http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/client.h
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 
operations.
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 ?


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

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


http://gerrit.cloudera.org:8080/#/c/3952/19/src/kudu/client/session-internal.cc
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  {
           NextBatcher();
          CanApplyWriteOp(&need_pre_flush);
} while (need_pre_flush)


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


http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.h
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