Alexey Serbin has posted comments on this change.

Change subject: KUDU-1752 C++ client error memory should be bounded
......................................................................


Patch Set 1:

(5 comments)

Thank you for the review.
 
> (4 comments)
 > 
 > Could you exercise the new code in some tests?

Certainly.  Probably, I should have marked the item as 'WIP' -- I wanted to 
collect some initial feedback first instead of writing a 'design doc' and 
asking for a feedback on that (just want to move quick on this).

 > 
 > 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.

That's a good idea.  I'm not sure it's realistic to implement and test that in 
a day or two.  Do you mind if I do that separately a little bit later?

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?
ok, this sounds like a good idea to me -- will fix.


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 i
Yep, this part I skipped partly because I was not sure what behavior we want 
here.

I think it's better to keep things simple and just report on error if the 
setting contradicts with current state of the error collector.  That's 
essentially the first approach you mentioned, but elaborated a little bit.  
I.e., allow to update on the memory limit if:
* the limit is increased
* the limit is decreased, but setting it would not make the collector de-facto 
overflown

Does it make sense to you?


PS1, Line 80:  else {
            :     STLDeleteElements(&errors_);
            :   }
> This is a slight change in semantics, though one that I agree with.
yep, that's just a way to send the accumulated errors into /dev/null

I think it makes sense to have such an ability.


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);
> What's the point of allowing this parameter to be set if it's never actuall
The idea was to have an interface where it's possible to limit this upon 
creation of an ErrorCollector instance.  I was playing with the case when the 
limit on the error buffer size comes from the parent parent object.

However, right now it's not used -- that's correct.  I'll remove this.


Line 38:   ErrorCollector(size_t max_mem_size_bytes = 0);
> warning: single-argument constructors must be marked explicit to avoid unin
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@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