Alexey Serbin has posted comments on this change. Change subject: KUDU-1752 C++ client error memory should be bounded ......................................................................
Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/5308/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS2, Line 4237: // Set the limit very low: not a single error can be added into the : // error collector buffer. > Isn't this duplicated in the comment below? Done PS2, Line 4278: has'nt > hasn't Done http://gerrit.cloudera.org:8080/#/c/5308/2/src/kudu/client/client-unittest.cc File src/kudu/client/client-unittest.cc: Line 29: #include "kudu/gutil/gscoped_ptr.h" > Not used. Done Line 178: namespace internal { > This is a little weird, could you prefix each reference to ErrorCollector w That's because I wanted the FRIEND_TEST() macro to in error_collector.h; that's not about just ErrorCollector usage in this code. OK, I'll just remove that macro, adding the would-be-result in there manually. http://gerrit.cloudera.org:8080/#/c/5308/2/src/kudu/client/client.h File src/kudu/client/client.h: PS2, Line 1519: the session > this session's Done PS2, Line 1522: the > Don't need Done PS2, Line 1524: Having the error buffer space limit set > If the error buffer space limit is set, Done PS2, Line 1525: varies. It depends on error conditions, : /// write operation type (insert/update/delete) and the workload's row sizes. > varies depending on error conditions, write operation types (insert/update/ Done PS2, Line 1528: the session starts dropping all the subsequent : /// errors along with the first error which would overflow the buffer, : /// if added > the session will drop the first error that would overflow the buffer as wel Done PS2, Line 1531: content > contents Done PS2, Line 1539: since last call : /// of the GetPendingErrors() method > "since the last call to the GetPendingErrors() method" Done PS2, Line 1541: the > Don't need Done PS2, Line 1565: storage > buffer Done http://gerrit.cloudera.org:8080/#/c/5308/2/src/kudu/client/error_collector.cc File src/kudu/client/error_collector.cc: Line 45: Status ErrorCollector::SetMaxMemSize(size_t size_bytes) { > What about the edge case you were telling me about, where there's not enoug That's covered by the updated code in ErrorCollector::CountErrors() and checks for Flush() status code. PS2, Line 53: Substitute > No actual substitution is happening here. Done PS2, Line 59: more than > Remove Done -- To view, visit http://gerrit.cloudera.org:8080/5308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f Gerrit-PatchSet: 2 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
