Alexey Serbin has posted comments on this change.

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


Patch Set 18:

(15 comments)

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

PS18, Line 522: Add
> Does it make sense to add a coment that callers are expected to serialize d
I think the original design of the Batch class is to accommodate multiple 
callers of this method.


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

Line 168: 
> typo: the the
Done


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


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 N
The data_ is allocated in the constructor of the Kudu::Client() object, so it 
should not be nullptr: if there is not enough memory, std::bad_alloc() should 
be thrown and it will be termination with SIGABRT due to the unexpected 
exception.


PS18, Line 736: int
> uint ?
Good point -- I added this to be in line with SetTimeoutMillis(), but using 
unit would be better.  Will change.


PS18, Line 740: int
> uint ?
This comes from the interface which is already published.  I'm not sure whether 
it's OK to change it in an incompatible way now.


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 ?
Yes, there is.  You could find it in the description for the AUTO_FLUSH_SYNC 
mode.


Line 1038:     /// batch is sent and the reclamed space is available for new 
operations.
> typo: reclamed
Done


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 
No, this is specific only for this mode.  In all other cases it's either 
possible to specify a callback or it's clear that the callback is not needed 
(because the flush operation is synchronous).


Line 1044:     ///   (probably the messenger IO threads?).
> I guess this todo is already addressed ?
Yes, this is clear that it's not needed now -- will remove.


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 ? :)
Yes -- this is to avoid circular dependencies later when NextBatcher is called. 
 It's possible to use shared_ptr here as well, but it should be transformed 
into weak_ptr in the batcher anyway.


PS19, Line 263: (
> Isn't this redundant ? We could as well place NextBatcher above ?
No, it's not.  Only in rare cases the additional pre-flush is needed (i.e. call 
of NextBatcher()).  Please read on the diagram which you liked.


PS19, Line 270: lock_
> I thought whole routine is serialized, so why another spinlock here ?
Because there is another actor -- the background 'periodic' flush task which 
calls NextBatcher() and replaces batcher_.


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 ?
Done


PS18, Line 165: cond_mutex_
> Trivial nit: I suggest to rename this to flow_control_cond_mutex_ which cou
Good idea -- thanks!

I'll rename it and leave the comment since it explains why this is different 
from the lock_ and gives info in which order it's necessary to acquire both, if 
needed (actually, the code never acquires both).


-- 
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: 18
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