Alexey Serbin has posted comments on this change.

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


Patch Set 23:

(2 comments)

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

Line 118:   // Thread-safety note: the buffer_bytes_limit_ is not supposed to 
be accessed
> I think it's important to have safety notes like this in the header so that
But from the method caller's perspective all what's needed to know is that it's 
not supposed to be called from multiple threads, plus generic info on the 
parameters.  I don't see more.

Am I missing something?

I can put notion regarding the thread-safety constraint into the header.  Would 
it be enough?


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

Line 191:   std::unordered_set<internal::Batcher*> flushed_batchers_;
> Isn't the count of bytes in currently pending operations kept in the buffer
Correct.

As a matter of fact, I've removed flished_batchers_ already in the patchset I'm 
preparing.  I think now it's looks better.  Probably, it would be nice to get 
some feedback on that from the original author of the Batcher class (Todd 
and/or David?).


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