[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

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


KUDU-1752 C++ client error memory should be bounded

Added KuduSession::SetErrorBufferSpace() method to set limit on maximum
size of memory consumed by session errors.  To preserve the legacy
behavior, a session is created with no limit on error memory size.

Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Reviewed-on: http://gerrit.cloudera.org:8080/5308
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/write_op.h
9 files changed, 206 insertions(+), 14 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5308

to look at the new patch set (#4).

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

KUDU-1752 C++ client error memory should be bounded

Added KuduSession::SetErrorBufferSpace() method to set limit on maximum
size of memory consumed by session errors.  To preserve the legacy
behavior, a session is created with no limit on error memory size.

Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/write_op.h
9 files changed, 206 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/5308/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5308

to look at the new patch set (#3).

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

KUDU-1752 C++ client error memory should be bounded

Added KuduSession::SetErrorsMaxMemSize() method to set limit on maximum
size of memory consumed by session errors.  To preserve the legacy
behavior, a session is created with no limit on error memory size.

Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/write_op.h
9 files changed, 206 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/5308/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Alexey Serbin 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?
Done


PS2, Line 4278: has'nt
> hasn't
Done


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


Line 178: namespace internal {
> This is a little weird, could you prefix each reference to ErrorCollector w
That's because I wanted the FRIEND_TEST() macro to in error_collector.h; that's 
not about just ErrorCollector usage in this code.

OK, I'll just remove that macro, adding the would-be-result in there manually.


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
Done


PS2, Line 1522: the
> Don't need
Done


PS2, Line 1524: Having the error buffer space limit set
> If the error buffer space limit is set,
Done


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/
Done


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 wel
Done


PS2, Line 1531: content
> contents
Done


PS2, Line 1539: since last call
  :   /// of the GetPendingErrors() method
> "since the last call to the GetPendingErrors() method"
Done


PS2, Line 1541: the
> Don't need
Done


PS2, Line 1565: storage
> buffer
Done


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 enoug
That's covered by the updated code in ErrorCollector::CountErrors() and checks 
for Flush() status code.


PS2, Line 53: Substitute
> No actual substitution is happening here.
Done


PS2, Line 59: more than
> Remove
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5308

to look at the new patch set (#2).

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

KUDU-1752 C++ client error memory should be bounded

Added KuduSession::SetErrorsMaxMemSize() method to set limit on maximum
size of memory consumed by session errors.  To preserve the legacy
behavior, a session is created with no limit on error memory size.

Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
---
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/write_op.h
9 files changed, 214 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/5308/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 1520:   /// By default, when a session is created, there is no limit on 
maximum size.
> can you comment on what happens when this limit is reached?
Done


Line 1521:   ///
> might be worth commenting that errors contain error messages that are poten
Done


Line 1524:   ///   where @c 0 means 'unlimited'.
> is negative also unlimited, or what happens?
The 'size_t' is an unsigned type, so I don't think talking about negative 
values is relevant here.  However, you make a good point: may be, it's worth to 
use a signed type instead.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

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


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 1524:   ///   where @c 0 means 'unlimited'.
> is negative also unlimited, or what happens?
size_t is unsigned.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(1 comment)

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

Absolutely; I didn't mean to suggest you should do it now. After all, it 
wouldn't change the client interface.

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) {
> A small addition: if the limit is increased, it's also necessary to make su
Your suggested approach seems fine to me, though we'll need to describe it in 
client.h too.

If you find the explanation to be too complex, I think dumbing down the 
implementation by prohibiting changes if there's even one error is fine too.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 1:

(3 comments)

Thanks!

Unit tests?

http://gerrit.cloudera.org:8080/#/c/5308/1/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 1520:   /// By default, when a session is created, there is no limit on 
maximum size.
can you comment on what happens when this limit is reached?


Line 1521:   ///
might be worth commenting that errors contain error messages that are 
potentially large (e.g. containing traces at some point), as well as the 
partial row. Depending on the error condition and the size of the row, the 
number of errors this handles may vary.


Line 1524:   ///   where @c 0 means 'unlimited'.
is negative also unlimited, or what happens?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 1:

(1 comment)

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) {
> Yep, this part I skipped partly because I was not sure what behavior we wan
A small addition: if the limit is increased, it's also necessary to make sure 
no errors dropped so far.  Otherwise the set of accumulated errors would 
contain a hole -- we don't want that, right?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

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(_);
:   }
> 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Adar Dembo (Code Review)]

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(_);
:   }
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1752 C++ client error memory should be bounded

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5308

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

KUDU-1752 C++ client error memory should be bounded

Added KuduSession::SetErrorsMaxMemSize() method to set limit on maximum
size of memory consumed by session errors.  To preserve the legacy
behavior, a session is created with no limit on error memory size.

Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/write_op.h
7 files changed, 59 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/5308/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b9fe3e83bab2a0b703b685ec7f3bb1db1601e5f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin