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/09/16 00:58:35 UTC
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4432
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
[c++client] added TestAutoFlushBackgroundDropSession
Added TestAutoFlushBackgroundDropSession to the client-test suite.
This is a follow-up for 1a062253e3fdc900a4b0b418520d2870b6de8846.
Change-Id: If136872a4200e18acbb587276fed28975afe3810
---
M src/kudu/client/client-test.cc
1 file changed, 43 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/4432/1
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4432/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2431: its pending data should not be flushed.
> Are you sure these are the semantics we want? They make sense for MANUAL_FL
Great observation!
We don't have requirements for that, but after some consideration I think it's worth discussing this, indeed.
From one hand, the behavior you mentioned would be more user-friendly and elegant from the coding perspective: knowing the session is run in AUTO_FLUSH_BACKGROUND mode it's not necessary to call Flush() in the very end of the 'logical' session.
From the other hand, changing the current behavior would require providing a means to report an error encountered during flushing the reminding operations, right? Otherwise, how could a user know that the operations of the abandoned session were flushed successfully?
That's said, I would consider declaring this a usability bug and addressing this issue in a separate changelist.
Would it be OK?
PS1, Line 2433: due
> Nit: due to
Done
Line 2446: SleepFor(MonoDelta::FromMilliseconds(3 * kFlushIntervalMs));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4432/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2431: its pending data should not be flushed.
> Yes, of course that doesn't need to be addressed here. Just something to th
Sure, I filed the following JIRA for that: https://issues.apache.org/jira/browse/KUDU-1621
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4432/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2431: its pending data should not be flushed.
> Great observation!
Yes, of course that doesn't need to be addressed here. Just something to think about (and perhaps to compare with the Java client).
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
[c++client] added TestAutoFlushBackgroundDropSession
As a follow-up for 1a062253e3fdc900a4b0b418520d2870b6de8846,
added new TestAutoFlushBackgroundDropSession test.
The test verifies that a session in AUTO_FLUSH_BACKGROUND mode can
be safely abandoned so its pending operations are not flushed.
Also, reduced number of write operations applied to the session and
updated the limit on mutation buffer size accordingly in
TestApplyTooMuchWithoutFlushing test to reduce running time for
ASAN/TSAN configurations. The semantics of the test is to try
adding more data into the mutation buffer than the limit on the buffer
size: the crux here is in the relative sizes, not the absolute ones.
Change-Id: If136872a4200e18acbb587276fed28975afe3810
Reviewed-on: http://gerrit.cloudera.org:8080/4432
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 43 insertions(+), 19 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4432/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2431: its pending data should not be flushed.
Are you sure these are the semantics we want? They make sense for MANUAL_FLUSH but in AUTO_FLUSH_BACKGROUND don't we want the session to linger in memory until all pending operations have been flushed?
I mean, knowing how it's implemented, I understand why this is the case, but I just wonder whether these are the desired semantics.
PS1, Line 2433: due
Nit: due to
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [c++client] added TestAutoFlushBackgroundDropSession
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/4432
to look at the new patch set (#2).
Change subject: [c++client] added TestAutoFlushBackgroundDropSession
......................................................................
[c++client] added TestAutoFlushBackgroundDropSession
As a follow-up for 1a062253e3fdc900a4b0b418520d2870b6de8846,
added new TestAutoFlushBackgroundDropSession test.
The test verifies that a session in AUTO_FLUSH_BACKGROUND mode can
be safely abandoned so its pending operations are not flushed.
Also, reduced number of write operations applied to the session and
updated the limit on mutation buffer size accordingly in
TestApplyTooMuchWithoutFlushing test to reduce running time for
ASAN/TSAN configurations. The semantics of the test is to try
adding more data into the mutation buffer than the limit on the buffer
size: the crux here is in the relative sizes, not the absolute ones.
Change-Id: If136872a4200e18acbb587276fed28975afe3810
---
M src/kudu/client/client-test.cc
1 file changed, 43 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/4432/2
--
To view, visit http://gerrit.cloudera.org:8080/4432
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If136872a4200e18acbb587276fed28975afe3810
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot