Adar Dembo has posted comments on this change. Change subject: KUDU-1752 C++ client error memory should be bounded ......................................................................
Patch Set 1: (4 comments) Could you exercise the new code in some tests? Separately, it may be interesting to use util/mem_tracker for client-side memory limiting and tracking. Both for this and for the batcher (maybe meta cache too). The effect should be more or less the same, but it comes with preexisting infrastructure for publishing metrics. http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/client.h File src/kudu/client/client.h: Line 1525: void SetErrorsMaxMemSize(size_t size_bytes); To mirror SetMutationBufferSpace, how about SetErrorBufferSpace? http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/error_collector.cc File src/kudu/client/error_collector.cc: Line 41: void ErrorCollector::SetMaxMemSize(size_t size_bytes) { Even though we don't expect to use it in this way, it would be nice if in isolation ErrorCollector::SetMaxMemSize() did the "right thing" when the memory size goes down. What is the "right thing"? 1) Do nothing (or return failure) if there's already a collected error. 2) Throw out a bunch of errors until the new limit is respected. PS1, Line 80: else { : STLDeleteElements(&errors_); : } This is a slight change in semantics, though one that I agree with. http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/error_collector.h File src/kudu/client/error_collector.h: Line 38: ErrorCollector(size_t max_mem_size_bytes = 0); > warning: single-argument constructors must be marked explicit to avoid unin What's the point of allowing this parameter to be set if it's never actually set? May as well restrict setting of max_mem_size_bytes_ in the setter. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[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
