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 2021/05/25 00:46:57 UTC

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default 

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


Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................

KUDU-2612: disable TxnSystemClient initialization by default

Currently the TxnSystemClient gets initialized on tablet servers by
default, retrying every second until successful, and logging an error
every minute if unable to. Since transactions support is currently
experimental and disabled by default, so too should this initialization
be. This patch hides the initialization with the existing
--disable_txn_system_client_init flag (previously used for tests), and
sets its default value to true.

Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_write_ops-itest.cc
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_service.cc
8 files changed, 86 insertions(+), 25 deletions(-)



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

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

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17499/3/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:

http://gerrit.cloudera.org:8080/#/c/17499/3/src/kudu/transactions/txn_system_client.cc@433
PS3, Line 433:     LOG(INFO) << "TxnSystemClient initialization is disabled...";
Thank you!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 25 May 2021 07:26:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

Overall looks good, just one quick question.

http://gerrit.cloudera.org:8080/#/c/17499/2/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:

http://gerrit.cloudera.org:8080/#/c/17499/2/src/kudu/transactions/txn_system_client.cc@432
PS2, Line 432:     // completes.
Does it make sense to log about this no-op which returns Status::OK()?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 25 May 2021 06:16:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17499/1/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:

http://gerrit.cloudera.org:8080/#/c/17499/1/src/kudu/transactions/txn_system_client.cc@58
PS1, Line 58: disable_txn_system_client_init
If we are going this route and this flag starts to appear not only in tests, maybe it's a good point to rename it to be similar to the majority of the flags with 'enable/disable' semantics?  What do you think about renaming this into 'txn_system_client_enable' or alike?  I guess the main thing is to have 'enable_something' instead of 'disable_something' -- that's a bit easier to read and understand.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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: Tue, 25 May 2021 01:29:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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/17499

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................

KUDU-2612: disable TxnSystemClient initialization by default

Currently the TxnSystemClient gets initialized on tablet servers by
default, retrying every second until successful, and logging an error
every minute if unable to. Since transactions support is currently
experimental and disabled by default, so too should this initialization
be. This patch hides the initialization with the existing
--disable_txn_system_client_init flag (previously used for tests),
adjusted to --enable_txn_system_client_init to match most of our
feature-gating flags.

Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/location_assignment-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/integration-tests/txn_write_ops-itest.cc
M src/kudu/tools/kudu-txn-cli-test.cc
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
14 files changed, 143 insertions(+), 32 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17499/1/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:

http://gerrit.cloudera.org:8080/#/c/17499/1/src/kudu/transactions/txn_system_client.cc@58
PS1, Line 58: enable_txn_system_client_init,
> If we are going this route and this flag starts to appear not only in tests
Makes sense. Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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, 25 May 2021 05:22:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................

KUDU-2612: disable TxnSystemClient initialization by default

Currently the TxnSystemClient gets initialized on tablet servers by
default, retrying every second until successful, and logging an error
every minute if unable to. Since transactions support is currently
experimental and disabled by default, so too should this initialization
be. This patch hides the initialization with the existing
--disable_txn_system_client_init flag (previously used for tests),
adjusted to --enable_txn_system_client_init to match most of our
feature-gating flags.

Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Reviewed-on: http://gerrit.cloudera.org:8080/17499
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/location_assignment-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/integration-tests/txn_write_ops-itest.cc
M src/kudu/tools/kudu-txn-cli-test.cc
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
14 files changed, 144 insertions(+), 32 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 25 May 2021 15:48:21 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2612: disable TxnSystemClient initialization by default

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

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

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

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

Change subject: KUDU-2612: disable TxnSystemClient initialization by default
......................................................................

KUDU-2612: disable TxnSystemClient initialization by default

Currently the TxnSystemClient gets initialized on tablet servers by
default, retrying every second until successful, and logging an error
every minute if unable to. Since transactions support is currently
experimental and disabled by default, so too should this initialization
be. This patch hides the initialization with the existing
--disable_txn_system_client_init flag (previously used for tests),
adjusted to --enable_txn_system_client_init to match most of our
feature-gating flags.

Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/location_assignment-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/integration-tests/txn_write_ops-itest.cc
M src/kudu/tools/kudu-txn-cli-test.cc
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
14 files changed, 144 insertions(+), 32 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c020a66db484f88ae1cb7c15d860d503a3f8a3b
Gerrit-Change-Number: 17499
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-Reviewer: Tidy Bot (241)