You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/12/01 19:01:12 UTC
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
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 <as...@cloudera.com>
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
---
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] KUDU-1752 C++ client error memory should be bounded
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot