Adar Dembo 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? PS2, Line 4278: has'nt hasn't 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. Line 178: namespace internal { This is a little weird, could you prefix each reference to ErrorCollector with internal:: instead? 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 PS2, Line 1522: the Don't need PS2, Line 1524: Having the error buffer space limit set If the error buffer space limit is set, 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/delete), and write operation row sizes. 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 well as all subsequent errors. PS2, Line 1531: content contents PS2, Line 1539: since last call : /// of the GetPendingErrors() method "since the last call to the GetPendingErrors() method" PS2, Line 1541: the Don't need PS2, Line 1565: storage buffer 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 enough room to store even one error, causing issues within the session apply/flush path? PS2, Line 53: Substitute No actual substitution is happening here. PS2, Line 59: more than Remove -- 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
