You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/08/07 01:15:50 UTC

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap 

Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16304


Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................

KUDU-2612 p8: replay participant ops on bootstrap

This patch adds the ability to bootstrap a Tablet's TxnParticipant from
its WALs. The replay is a bit crude, in that we'll always update state
based on replicate/commit pairs, foregoing the usual state checks. This
is acceptable because presumably this checking happened the first time
the participant ops went through replication.

This patch doesn't add any form of WAL anchoring. This will come in a
separate patch.

Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
---
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/ops/participant_op.cc
M src/kudu/tablet/ops/participant_op.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/txn_participant-test-util.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.h
8 files changed, 194 insertions(+), 72 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc
File src/kudu/integration-tests/txn_participant-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@193
PS1, Line 193: TEST_F(TxnParticipantITest, TestCopyParticipantOps) {
Do you mind adding a short doc blurb to summarize the essence of this test scenario?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@196
PS1, Line 196: const
nit: constexpr ?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@201
PS1, Line 201: MonoDelta::FromSeconds(10)
nit: separate this into a constant and use here and at line 230 ?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@207
PS1, Line 207: statuses
What is the significance of collecting these statuses?  I could not see that the test scenario uses it later on.


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@225
PS1, Line 225: 3
nit: maybe, replace with
  cluter_->num_tablet_servers()


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h
File src/kudu/tablet/ops/participant_op.h:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h@61
PS1, Line 61: ValidateOp()
It would nit nice to add a short doc blurb explaining the essence what kind of validation is performed here.


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h@61
PS1, Line 61: ValidateOp
Could ValidateOp() be a private method?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.cc@124
PS1, Line 124: txn->BeginTransaction()
nit here and elsewhere with successful cases: just return from the method?  Or the idea is that there will be some extra-code in the future as the note above explains?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc@291
PS1, Line 291: const
nit: constexpr ?


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc@303
PS1, Line 303: 
nit: remove the extra line?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 08 Aug 2020 04:46:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16304/2/src/kudu/integration-tests/txn_participant-itest.cc
File src/kudu/integration-tests/txn_participant-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16304/2/src/kudu/integration-tests/txn_participant-itest.cc@212
PS2, Line 212:  ASSERT_OK(s);
nit: if this failed, would it make sense to have SCOPED_TRACE or something to understand what transaction id and operation it was?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Aug 2020 02:52:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 13 Aug 2020 05:25:08 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16304/2/src/kudu/integration-tests/txn_participant-itest.cc
File src/kudu/integration-tests/txn_participant-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16304/2/src/kudu/integration-tests/txn_participant-itest.cc@212
PS2, Line 212:  SCOPED_TRACE(
> nit: if this failed, would it make sense to have SCOPED_TRACE or something 
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 12 Aug 2020 23:25:50 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................


Patch Set 1:

(10 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc
File src/kudu/integration-tests/txn_participant-itest.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@193
PS1, Line 193: TEST_F(TxnParticipantITest, TestCopyParticipantOps) {
> Do you mind adding a short doc blurb to summarize the essence of this test 
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@196
PS1, Line 196: const
> nit: constexpr ?
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@201
PS1, Line 201: MonoDelta::FromSeconds(10)
> nit: separate this into a constant and use here and at line 230 ?
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@207
PS1, Line 207: statuses
> What is the significance of collecting these statuses?  I could not see tha
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/integration-tests/txn_participant-itest.cc@225
PS1, Line 225: 3
> nit: maybe, replace with
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h
File src/kudu/tablet/ops/participant_op.h:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h@61
PS1, Line 61: ValidateOp()
> It would nit nice to add a short doc blurb explaining the essence what kind
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.h@61
PS1, Line 61: ValidateOp
> Could ValidateOp() be a private method?
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/ops/participant_op.cc@124
PS1, Line 124: txn->BeginTransaction()
> nit here and elsewhere with successful cases: just return from the method? 
Yeah, I was expected extra code in the future, but I can update this for now.


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:

http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc@291
PS1, Line 291: const
> nit: constexpr ?
Done


http://gerrit.cloudera.org:8080/#/c/16304/1/src/kudu/tablet/txn_participant-test.cc@303
PS1, Line 303: 
> nit: remove the extra line?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 08 Aug 2020 08:12:55 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

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

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

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

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................

KUDU-2612 p8: replay participant ops on bootstrap

This patch adds the ability to bootstrap a Tablet's TxnParticipant from
its WALs. The replay is a bit crude, in that we'll always update state
based on replicate/commit pairs, foregoing the usual state checks. This
is acceptable because presumably this checking happened the first time
the participant ops went through replication.

This patch doesn't add any form of WAL anchoring. This will come in a
separate patch.

Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
---
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/ops/participant_op.cc
M src/kudu/tablet/ops/participant_op.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/txn_participant-test-util.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.h
8 files changed, 194 insertions(+), 71 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

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

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

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

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................

KUDU-2612 p8: replay participant ops on bootstrap

This patch adds the ability to bootstrap a Tablet's TxnParticipant from
its WALs. The replay is a bit crude, in that we'll always update state
based on replicate/commit pairs, foregoing the usual state checks. This
is acceptable because presumably this checking happened the first time
the participant ops went through replication.

This patch doesn't add any form of WAL anchoring. This will come in a
separate patch.

Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
---
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/ops/participant_op.cc
M src/kudu/tablet/ops/participant_op.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/txn_participant-test-util.h
M src/kudu/tablet/txn_participant-test.cc
7 files changed, 194 insertions(+), 64 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16304 )

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................

KUDU-2612 p8: replay participant ops on bootstrap

This patch adds the ability to bootstrap a Tablet's TxnParticipant from
its WALs. The replay is a bit crude, in that we'll always update state
based on replicate/commit pairs, foregoing the usual state checks. This
is acceptable because presumably this checking happened the first time
the participant ops went through replication.

This patch doesn't add any form of WAL anchoring. This will come in a
separate patch.

Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Reviewed-on: http://gerrit.cloudera.org:8080/16304
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/ops/participant_op.cc
M src/kudu/tablet/ops/participant_op.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/txn_participant-test-util.h
M src/kudu/tablet/txn_participant-test.cc
7 files changed, 192 insertions(+), 64 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2612 p8: replay participant ops on bootstrap

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

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

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

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

Change subject: KUDU-2612 p8: replay participant ops on bootstrap
......................................................................

KUDU-2612 p8: replay participant ops on bootstrap

This patch adds the ability to bootstrap a Tablet's TxnParticipant from
its WALs. The replay is a bit crude, in that we'll always update state
based on replicate/commit pairs, foregoing the usual state checks. This
is acceptable because presumably this checking happened the first time
the participant ops went through replication.

This patch doesn't add any form of WAL anchoring. This will come in a
separate patch.

Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
---
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/ops/participant_op.cc
M src/kudu/tablet/ops/participant_op.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/txn_participant-test-util.h
M src/kudu/tablet/txn_participant-test.cc
7 files changed, 192 insertions(+), 64 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I199ed01c2244d16ed6fd7ded063e4c71f3c409ff
Gerrit-Change-Number: 16304
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)