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 <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

Reply via email to