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 2021/05/15 06:17:52 UTC
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17452
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
KUDU-2612 automatically flush sessions on txn commit (Java client)
With this patch, all transactional sessions created off a transction
handle are automatically flushed upon calling commit() on the handle.
As for the KuduTransaction.startCommit() method, it's now necessary
to flush all the transactional sessions created off the transaction
handle before calling the method, otherwise a NonRecoverableException
based on Status.Incomplete() is thrown.
This patch also contains several test scenarios for the newly introduced
functionality.
This changelist is a Java client's counterpart for 45ca93f0e.
Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
2 files changed, 445 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/17452/1
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 May 2021 21:58:17 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java:
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@520
PS1, Line 520: throw new NonRecoverableException(Status.Incomplete(String.format(
> Will this prevent the other sessions from being flushed? Is that okay?
Yep, this prevents the other sessions from being flushed. If necessary, the user of the API can handle the errors and retry committing (there is a test case added for that).
The same behavior we now have in the C++ client as well. If continuing flushing other sessions, there might be more errors, and the reported error would be harder to parse and handle.
If we want to continue flushing sessions regardless of error encountered, I think we can do that as well. However, I'd rather post that as a separate changelist given the C++ client already behaves the same as this code.
What do you think?
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@530
PS1, Line 530: if (s.hasPendingOperations()) {
> I think the idea is to do as little work with the somewhat wait-less versio
Yes, as Andrew mentioned, the idea is to keep this method as lightweight as possible, tracking only the result of transitioning into the BEGIN_COMMIT phase. Since the rest is taken care of the backend and is supposed to almost always complete successfully, the only piece left is making sure the sessions flush successfully, but that process might be heavy and take long time, especially in case of busy clusters.
In theory, we could go with the approach of 'chain of futures' or 'chain of callbacks', but this API is more oriented for sync-style use cases to be simple to use and reason about.
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 May 2021 17:57:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 2: Code-Review+2
LGTM, though probably worth getting Grant's thoughts since he also raised some questions.
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 May 2021 19:29:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java:
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@530
PS1, Line 530: if (s.hasPendingOperations()) {
> Is there a reason not to just flush here instead? similar to the WAIT_FOR_C
I think the idea is to do as little work with the somewhat wait-less version of the commit, and all the flushes might be a fairly heavy-weight operation.
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 May 2021 01:11:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17452
to look at the new patch set (#2).
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
KUDU-2612 automatically flush sessions on txn commit (Java client)
With this patch, all transactional sessions created off a transction
handle are automatically flushed upon calling commit() on the handle.
As for the KuduTransaction.startCommit() method, it's now necessary
to flush all the transactional sessions created off the transaction
handle before calling the method, otherwise a NonRecoverableException
based on Status.Incomplete() is thrown.
This patch also contains several test scenarios for the newly introduced
functionality.
This changelist is a Java client's counterpart for 45ca93f0e.
Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
2 files changed, 445 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/17452/2
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 2: Verified+1
unrelated UBSAN report in TxnCommitITest.TestLoadTxnStatusManagerWhenNoMasters
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 May 2021 19:05:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
KUDU-2612 automatically flush sessions on txn commit (Java client)
With this patch, all transactional sessions created off a transction
handle are automatically flushed upon calling commit() on the handle.
As for the KuduTransaction.startCommit() method, it's now necessary
to flush all the transactional sessions created off the transaction
handle before calling the method, otherwise a NonRecoverableException
based on Status.Incomplete() is thrown.
This patch also contains several test scenarios for the newly introduced
functionality.
This changelist is a Java client's counterpart for 45ca93f0e.
Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Reviewed-on: http://gerrit.cloudera.org:8080/17452
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
2 files changed, 445 insertions(+), 3 deletions(-)
Approvals:
Alexey Serbin: Verified
Andrew Wong: Looks good to me, approved
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 automatically flush sessions on txn commit (Java client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17452 )
Change subject: KUDU-2612 automatically flush sessions on txn commit (Java client)
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java:
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@520
PS1, Line 520: throw new NonRecoverableException(Status.Incomplete(String.format(
Will this prevent the other sessions from being flushed? Is that okay?
http://gerrit.cloudera.org:8080/#/c/17452/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@530
PS1, Line 530: if (s.hasPendingOperations()) {
Is there a reason not to just flush here instead? similar to the WAIT_FOR_COMPLETION branch?
--
To view, visit http://gerrit.cloudera.org:8080/17452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c52578dd736906cf2610c8cc58496381a9b73ec
Gerrit-Change-Number: 17452
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 16 May 2021 02:38:22 +0000
Gerrit-HasComments: Yes