You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2021/05/04 22:56:19 UTC

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements 

Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17401


Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................

[master] KUDU-2181 Unhide master change config flag & other improvements

This change:
- Unhides and turns the master Raft change config flag ON by default
- Updates the description of flags that are not passed to the new
  master
- Turns off log buffering on bringing up the new master to help
  debug any issues in startup of the new master

Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
---
M src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master_service.cc
M src/kudu/tools/tool_action_master.cc
3 files changed, 22 insertions(+), 21 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/master/master_service.cc@101
PS2, Line 101: TAG_FLAG(master_support_change_config, evolving);
nit: a flag is tagged 'evolving' by default if not marked 'stable' or 'experimental', so unless you want to point at that specifically, this line might be omitted


http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc@76
PS2, Line 76: 8
I recall during one presentation this was set to something higher, like 30 seconds because of corresponding timeout to wait for clock synchronization.  Does it make sense to set it to 30 seconds by default?


http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc@175
PS2, Line 175:   // In case of an error in bringing up the master we want to ensure the last log lines
             :   // are emitted to help debug the issue. Hence turn off log buffering.
             :   flags.emplace_back("--logbuflevel=-1");
BTW, what about --log_async flag for the newly started master?  In addition to glog flags, kudu server processes also utilize async logger: src/kudu/util/logging.{h,cc}



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 17:28:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

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

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

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................

[master] KUDU-2181 Unhide master change config flag & other improvements

This change:
- Unhides and turns the master Raft change config flag ON by default
- Updates the description of flags that are not passed to the new
  master
- Uses sync logging on bringing up the new master to help
  debug any issues in startup of the new master
- Updates the wait timeout while bringing up a new master to account
  for the higher ntp wait timeout. This timeout is also used for
  checking whether the new master's system catalog has caught up
  from WAL. So this'll bump up timeout for that case as well. Thought
  about introducing separate wait timeout for that case but felt like
  exposing too many knobs and user shouldn't care whether time
  is being taken for master bringup or catchup from WAL.

Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
---
M src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master_service.cc
M src/kudu/tools/tool_action_master.cc
3 files changed, 27 insertions(+), 23 deletions(-)


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

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

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2: Verified+1

> Patch Set 2: Verified-1
> 
> Build Failed 
> 
> http://jenkins.kudu.apache.org/job/kudu-gerrit/23817/ : FAILURE

Unrelated test failure in AdminCliTest.TestUnsafeChangeConfigForConfigWithTwoNodes


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 May 2021 21:28:52 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2:

Ping! Could I get a review please?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 16:55:10 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 3: Verified+1 -Code-Review


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 21:47:28 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................

[master] KUDU-2181 Unhide master change config flag & other improvements

This change:
- Unhides and turns the master Raft change config flag ON by default
- Updates the description of flags that are not passed to the new
  master
- Uses sync logging on bringing up the new master to help
  debug any issues in startup of the new master
- Updates the wait timeout while bringing up a new master to account
  for the higher ntp wait timeout. This timeout is also used for
  checking whether the new master's system catalog has caught up
  from WAL. So this'll bump up timeout for that case as well. Thought
  about introducing separate wait timeout for that case but felt like
  exposing too many knobs and user shouldn't care whether time
  is being taken for master bringup or catchup from WAL.

Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Reviewed-on: http://gerrit.cloudera.org:8080/17401
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Bankim Bhavsar <ba...@cloudera.com>
---
M src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master_service.cc
M src/kudu/tools/tool_action_master.cc
3 files changed, 27 insertions(+), 23 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Bankim Bhavsar: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has removed a vote on this change.

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/17401
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/master/master_service.cc@101
PS2, Line 101: TAG_FLAG(master_support_change_config, evolving);
> nit: a flag is tagged 'evolving' by default if not marked 'stable' or 'expe
Thanks for pointing that out. I thought stable is the default.


http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:

http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc@76
PS2, Line 76: 8
> I recall during one presentation this was set to something higher, like 30 
Thanks for bringing that up!


http://gerrit.cloudera.org:8080/#/c/17401/2/src/kudu/tools/tool_action_master.cc@175
PS2, Line 175:   // In case of an error in bringing up the master we want to ensure the last log lines
             :   // are emitted to help debug the issue. Hence turn off log buffering.
             :   flags.emplace_back("--logbuflevel=-1");
> BTW, what about --log_async flag for the newly started master?  In addition
That's better. Done.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 19:29:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 21:15:40 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2:

> Patch Set 2: Code-Review+2
> 
> In the future, please add people as reviewers if you want a review, or ping specific team members. I do subscribe to the reviews mailing list, but I don't open every email to check the contents.

Sure. Will do. Thanks for the review.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 17:22:11 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has removed a vote on this change.

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/17401
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 3: Code-Review+1

Unrelated test failure in TSAN:
ClientTest.ClearCacheAndConcurrentWorkload
BlockManagerType/TsRecoveryITest.TestChangeMaxCellSize/1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 21:47:14 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2181 Unhide master change config flag & other improvements

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

Change subject: [master] KUDU-2181 Unhide master change config flag & other improvements
......................................................................


Patch Set 2: Code-Review+2

In the future, please add people as reviewers if you want a review, or ping specific team members. I do subscribe to the reviews mailing list, but I don't open every email to check the contents.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1fc1dc5985601158ae58d5d190a60c1e542ea1d
Gerrit-Change-Number: 17401
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 14 May 2021 17:20:57 +0000
Gerrit-HasComments: No