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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
