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/03/23 21:13:12 UTC
[kudu-CR] [master] Script to automate adding a master
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17220
Change subject: [master] Script to automate adding a master
......................................................................
[master] Script to automate adding a master
This script is a culmination of the changes that have been made so far
to add a master to a Kudu cluster.
This script must be run from the master being added and orchestrates
the necessary workflow with a bunch of verification.
This script will allow significantly simplifying the documentation for
adding a master to the cluster.
TODO:
- Test on a real cluster and MacOS
- Add more negative test cases or convert existing ones.
Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/dynamic_multi_master-test.cc
A src/kudu/scripts/add_master.sh
3 files changed, 538 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/17220/1
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [master] Script to automate adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Patch Set 1:
> Patch Set 1: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/23535/ : FAILURE
Looks like the build machines don't have "bc" which is used to compare floating point numbers in bash. I'll look for alternative or some other technique.
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 24 Mar 2021 04:35:05 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Script to automate adding a master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Patch Set 3:
> > Patch Set 2:
> >
> > > Patch Set 1:
> > >
> > > > Patch Set 1:
> > > >
> > > > > Patch Set 1: Verified-1
> > > > >
> > > > > Build Failed
> > > > >
> > > > > http://jenkins.kudu.apache.org/job/kudu-gerrit/23535/ :
> FAILURE
> > > >
> > > > Looks like the build machines don't have "bc" which is used
> to compare floating point numbers in bash. I'll look for
> alternative or some other technique.
> > >
> > > This is getting to be a pretty hefty script. Before trying to
> jump through more hoops to get this working, have you given much
> thought to folding these calls into the tool as some `kudu master
> orchestrate_add_master` or even into the `add_master` tool
> directly?
> > >
> > > I'm fine with this being a bash script, but I want to be sure
> bash is actually the right tool for the job, especially considering
> we have a means to bake a lot of this into tooling fairly easily --
> heck, you've already done a lot of this in dynamic_multi_master-test!
> And from a maintainability perspective, big bash scripts like seem
> much more palatable when not written in bash :)
> >
> > If you look at series of recent changes most of them I've tried
> to push into the CLI tool or the server implementation itself.
> >
> > Current tooling doesn't have good means for process mgmt.
> ExternalMiniCluster does have that but it's not available for
> production tool as I see it. That's where bash is better.
> >
> > The script might be lengthy but I've used linters and tried to
> make it easier to read and maintain as much as possible.
> >
> > That being said, I'll look into more validations that could be
> put into the CLI tool or the server instead of the script. I'll
> explore whether more if not all could be migrated into the C++
> tool.
>
> Yeah, process management can be tough, but at the very least there
> are some utilities in util/subprocess.h that we use to run the
> tools in tests. I imagine the tricky part though is starting a
> Master server, but even that seems like something we could work
> around, especially if the end result of the tool is the new master
> is down, and we expect CM to start the master up, which I think
> would be the case anyway.
>
> It's your call in the end, just thought I'd raise the suggestion
> because the script was getting pretty complex, and at least most of
> the tools we have in the C++ codebase seem to overlap with what's
> needed here.
+1
I'd vote for CLI-based approach here as well, if possible. If not everything, then maybe create basic building blocks as commands in the CLI and use the scripting approach as the last resport just to glue all of these together?
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
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: Mon, 29 Mar 2021 18:15:49 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Script to automate adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Patch Set 2:
> Patch Set 1:
>
> > Patch Set 1:
> >
> > > Patch Set 1: Verified-1
> > >
> > > Build Failed
> > >
> > > http://jenkins.kudu.apache.org/job/kudu-gerrit/23535/ : FAILURE
> >
> > Looks like the build machines don't have "bc" which is used to compare floating point numbers in bash. I'll look for alternative or some other technique.
>
> This is getting to be a pretty hefty script. Before trying to jump through more hoops to get this working, have you given much thought to folding these calls into the tool as some `kudu master orchestrate_add_master` or even into the `add_master` tool directly?
>
> I'm fine with this being a bash script, but I want to be sure bash is actually the right tool for the job, especially considering we have a means to bake a lot of this into tooling fairly easily -- heck, you've already done a lot of this in dynamic_multi_master-test! And from a maintainability perspective, big bash scripts like seem much more palatable when not written in bash :)
If you look at series of recent changes most of them I've tried to push into the CLI tool or the server implementation itself.
Current tooling doesn't have good means for process mgmt. ExternalMiniCluster does have that but it's not available for production tool as I see it. That's where bash is better.
The script might be lengthy but I've used linters and tried to make it easier to read and maintain as much as possible.
That being said, I'll look into more validations that could be put into the CLI tool or the server instead of the script. I'll explore whether more if not all could be migrated into the C++ tool.
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
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: Wed, 24 Mar 2021 21:03:58 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Script to automate adding a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > Patch Set 1: Verified-1
> >
> > Build Failed
> >
> > http://jenkins.kudu.apache.org/job/kudu-gerrit/23535/ : FAILURE
>
> Looks like the build machines don't have "bc" which is used to compare floating point numbers in bash. I'll look for alternative or some other technique.
This is getting to be a pretty hefty script. Before trying to jump through more hoops to get this working, have you given much thought to folding these calls into the tool as some `kudu master orchestrate_add_master` or even into the `add_master` tool directly?
I'm fine with this being a bash script, but I want to be sure bash is actually the right tool for the job, especially considering we have a means to bake a lot of this into tooling fairly easily -- heck, you've already done a lot of this in dynamic_multi_master-test! And from a maintainability perspective, big bash scripts like seem much more palatable when not written in bash :)
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
Gerrit-PatchSet: 1
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: Wed, 24 Mar 2021 05:38:47 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Script to automate adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17220
to look at the new patch set (#3).
Change subject: [master] Script to automate adding a master
......................................................................
[master] Script to automate adding a master
This script is a culmination of the changes that have been made so far
to add a master to a Kudu cluster.
This script must be run from the master being added and orchestrates
the necessary workflow with a bunch of verification.
This script will allow significantly simplifying the documentation for
adding a master to the cluster.
TODO:
- Test on a real cluster and MacOS
- Add more negative test cases or convert existing ones.
Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/dynamic_multi_master-test.cc
A src/kudu/scripts/add_master.sh
3 files changed, 537 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/17220/3
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
Gerrit-PatchSet: 3
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)
[kudu-CR] [master] Script to automate adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has abandoned this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Abandoned
Moving the orchestration logic to the CLI tool.
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
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] Script to automate adding a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17220 )
Change subject: [master] Script to automate adding a master
......................................................................
Patch Set 3:
> Patch Set 2:
>
> > Patch Set 1:
> >
> > > Patch Set 1:
> > >
> > > > Patch Set 1: Verified-1
> > > >
> > > > Build Failed
> > > >
> > > > http://jenkins.kudu.apache.org/job/kudu-gerrit/23535/ : FAILURE
> > >
> > > Looks like the build machines don't have "bc" which is used to compare floating point numbers in bash. I'll look for alternative or some other technique.
> >
> > This is getting to be a pretty hefty script. Before trying to jump through more hoops to get this working, have you given much thought to folding these calls into the tool as some `kudu master orchestrate_add_master` or even into the `add_master` tool directly?
> >
> > I'm fine with this being a bash script, but I want to be sure bash is actually the right tool for the job, especially considering we have a means to bake a lot of this into tooling fairly easily -- heck, you've already done a lot of this in dynamic_multi_master-test! And from a maintainability perspective, big bash scripts like seem much more palatable when not written in bash :)
>
> If you look at series of recent changes most of them I've tried to push into the CLI tool or the server implementation itself.
>
> Current tooling doesn't have good means for process mgmt. ExternalMiniCluster does have that but it's not available for production tool as I see it. That's where bash is better.
>
> The script might be lengthy but I've used linters and tried to make it easier to read and maintain as much as possible.
>
> That being said, I'll look into more validations that could be put into the CLI tool or the server instead of the script. I'll explore whether more if not all could be migrated into the C++ tool.
Yeah, process management can be tough, but at the very least there are some utilities in util/subprocess.h that we use to run the tools in tests. I imagine the tricky part though is starting a Master server, but even that seems like something we could work around, especially if the end result of the tool is the new master is down, and we expect CM to start the master up, which I think would be the case anyway.
It's your call in the end, just thought I'd raise the suggestion because the script was getting pretty complex, and at least most of the tools we have in the C++ codebase seem to overlap with what's needed here.
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
Gerrit-PatchSet: 3
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: Wed, 24 Mar 2021 21:22:05 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Script to automate adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17220
to look at the new patch set (#2).
Change subject: [master] Script to automate adding a master
......................................................................
[master] Script to automate adding a master
This script is a culmination of the changes that have been made so far
to add a master to a Kudu cluster.
This script must be run from the master being added and orchestrates
the necessary workflow with a bunch of verification.
This script will allow significantly simplifying the documentation for
adding a master to the cluster.
TODO:
- Test on a real cluster and MacOS
- Add more negative test cases or convert existing ones.
Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/dynamic_multi_master-test.cc
A src/kudu/scripts/add_master.sh
3 files changed, 537 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/17220/2
--
To view, visit http://gerrit.cloudera.org:8080/17220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc9dad73be625ff30070c522626bd79f1f1aa90c
Gerrit-Change-Number: 17220
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)