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