You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/04/23 19:02:07 UTC

[kudu-CR] transaction: avoid locking for TransactionState::timestamp 

Hello Andrew Wong,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
---
M src/kudu/tablet/transactions/transaction.h
1 file changed, 9 insertions(+), 8 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc
File src/kudu/tablet/ops/op.cc:

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc@35
PS5, Line 35: timestamp_(Timestamp())
> Ah, probably that's not enough due to those failures in WebserverCrawlITest
yea it seems libc++ doesn't call the default constructor whereas libstdcxx does. This will be fixed in C++20 apparently.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 May 2020 22:50:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

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

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

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
---
M src/kudu/common/timestamp.h
M src/kudu/tablet/ops/op.cc
M src/kudu/tablet/ops/op.h
3 files changed, 14 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15791/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15791 )

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
---
M src/kudu/tablet/ops/op.cc
M src/kudu/tablet/ops/op.h
2 files changed, 12 insertions(+), 11 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc
File src/kudu/tablet/ops/op.cc:

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc@35
PS5, Line 35: timestamp_(Timestamp())
> nit: this is not necessary after the changes in the signature of the Timest
Ah, probably that's not enough due to those failures in WebserverCrawlITest.TestAllWebPages under TSAN.

OK, that's interesting.

Anyways, thank you for the new revision with the fix!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 May 2020 21:57:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 1: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 25 Apr 2020 01:42:24 +0000
Gerrit-HasComments: No

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

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

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

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
---
M src/kudu/tablet/ops/op.h
1 file changed, 9 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15791/1/src/kudu/tablet/transactions/transaction.h
File src/kudu/tablet/transactions/transaction.h:

http://gerrit.cloudera.org:8080/#/c/15791/1/src/kudu/tablet/transactions/transaction.h@289
PS1, Line 289: Howeer
However



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 25 Apr 2020 01:26:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc
File src/kudu/tablet/ops/op.cc:

http://gerrit.cloudera.org:8080/#/c/15791/5/src/kudu/tablet/ops/op.cc@35
PS5, Line 35: timestamp_(Timestamp())
nit: this is not necessary after the changes in the signature of the Timestamp() constructors, but it doesn't hurt



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 May 2020 21:52:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 May 2020 17:46:18 +0000
Gerrit-HasComments: No

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................


Patch Set 1: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 25 Apr 2020 00:58:06 +0000
Gerrit-HasComments: No

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

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

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Reviewed-on: http://gerrit.cloudera.org:8080/15791
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/common/timestamp.h
M src/kudu/tablet/ops/op.cc
M src/kudu/tablet/ops/op.h
3 files changed, 14 insertions(+), 13 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] transaction: avoid locking for TransactionState::timestamp

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15791 )

Change subject: transaction: avoid locking for TransactionState::timestamp_
......................................................................

transaction: avoid locking for TransactionState::timestamp_

This field is already synchronized externally for mutations by the transaction
lifecycle (we never have a case where multiple threads are trying to
concurrently assign a timestamp to a transaction). The timestamp might
be concurrently read (eg by ToString() for the /transactions web page)
but an atomic variable is sufficient for this use case.

I saw a couple percent of the Apply thread's CPU going to managing this
locking, which should go away with this change.

Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
---
M src/kudu/common/timestamp.h
M src/kudu/tablet/ops/op.cc
M src/kudu/tablet/ops/op.h
3 files changed, 13 insertions(+), 13 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec
Gerrit-Change-Number: 15791
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>