Add quorum commands to CLI by Xenorith · Pull Request #17570 · Alluxio/alluxio

Xenorith · 2023-06-07T20:14:21Z

Add quorum/HA related commands as part of #17522

Xenorith · 2023-06-07T20:15:09Z

cli/src/alluxio.org/cli/cmd/quorum/elect.go

+return c.Run(nil)
+},
+})
+// TODO: to be consistent with other quorum commands, should this have a domain flag?


@jenoudet job master isn't a true quorum so there's no election, but should this still take in a domain flag with MASTER being the only accepted value for now?

The bash version of the command does not take a -domain parameter, and only operates on the MASTER domain. I think the go version of the command should work the same.

i understand that's how the bash version works and not aiming to change anything for this PR, but for the future, i'd like to know if my statement is true which implies this command should also be domain specific. otherwise someone would wonder if the elect command applies to all domains which also makes no sense, thus leading to confusion

Maybe it's worth adding a domain parameter in the future to keep things consistent. That would require implementing further logic on the Java side, but not a huge deal.

i'd like to add it to the go side to maintain consistency even though it will not be passed along to the java command., so i'd be adding a required flag --domain with MASTER being the only accepted value. it sets the stage for future changes without any significant impact besides forcing the user to know which domain they are triggering (which i say is a good thing)

Sounds good!

jenoudet · 2023-06-07T20:46:47Z

cli/src/alluxio.org/cli/cmd/quorum/info.go

+},
+})
+const domain = "domain"
+cmd.Flags().StringVar(&c.Domain, domain, "", "")


Add a description saying it must be either "MASTER" or "JOB_MASTER".

jenoudet · 2023-06-07T20:49:20Z

cli/src/alluxio.org/cli/cmd/quorum/elect.go

+return c.Run(nil)
+},
+})
+// TODO: to be consistent with other quorum commands, should this have a domain flag?


The bash version of the command does not take a -domain parameter, and only operates on the MASTER domain. I think the go version of the command should work the same.

jenoudet · 2023-06-07T20:51:40Z

cli/src/alluxio.org/cli/cmd/quorum/remove.go

+},
+})
+const address, domain = "address", "domain"
+cmd.Flags().StringVar(&c.Domain, domain, "", "")


Add description about possible domains as well, same as for info.

jenoudet

We could start implementing the validation using the validator package. I've left some comments and examples of its use. wdyt?

jenoudet · 2023-06-07T22:53:03Z

cli/src/alluxio.org/cli/cmd/quorum/elect.go


 func (c *ElectCommand) Run(_ []string) error {
-return c.Base().Run([]string{"elect", "-address", c.ServerAddress})
+if err := checkDomain(c.QuorumCommand.domain, c.QuorumCommand.allowedDomains...); err != nil {


for validation I think a better tool would be https://github.com/go-playground/validator.
you can use the oneof validator https://pkg.go.dev/github.com/go-playground/validator/v10#hdr-One_Of
this would then look like

if err := validator.New().Struct(c); err != nil { return stacktrace.Propagate ... }

jenoudet · 2023-06-07T22:55:54Z

cli/src/alluxio.org/cli/cmd/quorum/quorum.go

+*env.BaseJavaCommand
+
+allowedDomains []string
+domain string


Using validator.

Suggested change

domain string

domain string `validate:"oneof=MASTER JOB_MASTER"`

jenoudet · 2023-06-07T23:00:18Z

cli/src/alluxio.org/cli/cmd/quorum/elect.go


-func (c *ElectCommand) Base() *env.BaseJavaCommand {
-return c.BaseJavaCommand
+serverAddress string


can use the hostname_port validator for this as well

Suggested change

serverAddress string

serverAddress string `validate:"hostname_port"`

jenoudet · 2023-06-07T23:01:31Z

cli/src/alluxio.org/cli/cmd/quorum/remove.go


-func (c *RemoveCommand) Base() *env.BaseJavaCommand {
-return c.BaseJavaCommand
+serverAddress string


With validator.

Suggested change

serverAddress string

serverAddress string `validate:"hostname_port"`

Xenorith

i would consider this as an improvement along with many others listed in the parent issue: #17522

as it is a nice to have, i don't plan on incorporating this in the initial iteration. adding it deserves a thorough review of all the commands to see where it can be used. in the expected case, it should save a lot of extra fluff lines. in the worst case, it can only validate a small percentage of the flags due to some convoluted logic of ours; then we may consider to not utilize it.

Xenorith · 2023-06-08T04:01:08Z

alluxio-bot, merge this please

maobaolong · 2023-06-12T01:08:17Z

@Xenorith Would you like to add a golang version ratis-shell into apache/ratis repository?

Xenorith · 2023-06-13T04:21:20Z

@Xenorith Would you like to add a golang version ratis-shell into apache/ratis repository?

@maobaolong i took a very quick glance and i didn't see as strong of a need for a golang version of the shell, but i'm open to hearing more about the pains that could be solved. i can provide a similar framework and template at the very least

Add quorum/HA related commands as part of #17522 pr-link: #17570 change-id: cid-e9c76998b8fbfd023e0898c876cc495801a87fc5

Add quorum/HA related commands as part of Alluxio#17522 pr-link: Alluxio#17570 change-id: cid-e9c76998b8fbfd023e0898c876cc495801a87fc5

- Add cli/ folder containing golang code - Add script to compile code to executable in build/cli/build-cli.sh - Add entrypoint script for development in bin/cli.sh - Add build profile to build the CLI as part of tha maven build via `-PgoCli` - Add `conf` service, consisting of - `bin/cli.sh conf get` as an example command, equivalent to `bin/alluxio getConf` - `bin/cli.sh conf log --name fully.qualified.class.path`, equivalent to `bin/alluxio fsadmin logLevel --logName fully.qualified.class.path` See #17522 for more background info pr-link: #17532 dev docs for defining conventions for the new alluxio cli in golang pr-link: #17530 - Add commands to start/stop individual processes, ex. `bin/cli.sh process start master` - Define process interface and registry - process specific java opts are defined with the process and are dynamically added to env - Add journal format command as part of starting master process - Mount options for worker are not ported because they will be deprecated in the near future pr-link: #17561 Adds `info` subcommand, which contains: - `cache`: worker capacity information, calls the existing `bin/alluxio fsadmin report capacity` command - `collect`: collects cluster information into a single tarball, calls the existing `bin/alluxio collectInfo` command - `master`: master quorum information, calls the existing `bin/alluxio fs masterInfo` command - `report`: cluster information, calls the existing `bin/alluxio fsadmin report` command, excluding the `capacity` category pr-link: #17566 Add journal commands to CLI as part of #17522 pr-link: #17569 Add quorum/HA related commands as part of #17522 pr-link: #17570 Add fs commands to golang CLI as part of #17522 unlike other CLI commands, utilize arguments to maintain the existing structure of filesystem commands (ex. cp, du, ls, mv, rm, etc) pr-link: #17580 Add generate commands to golang CLI as part of #17522 bin/alluxio docGen -> bin/cli generate docs bin/alluxio bootstrapConf -> bin/cli generate template to read ALLUXIO_CONF_DIR, also conducted a minor change on env variables. pr-link: #17790 Add exec commands to golang CLI as part of #17522 `bin/alluxio runClass` -> `bin/cli exec class` `bin/alluxio runTests` -> `bin/cli exec testRun` `bin/alluxio runUfsTests` -> `bin/cli exec testUfs` `bin/alluxio runUfsIOTest` -> `bin/cli exec testUfsIO` `bin/alluxio runHdfsMountTests` -> `bin/cli exec testHdfsMount` `bin/alluxio runHmsTests` -> `bin/cli exec testHms` `bin/alluxio runJournalCrashTest` -> `bin/cli exec testJournalCrash` For command `exec test`, using different `--name` flags can lead to huge difference in other flags and options. To manage flags and options, currently using different commands for different test types. pr-link: #17797 Prepare golang cli for tarball build and replace bin/alluxio allow java 8 or 11 add cli binaries to other tarball profiles Add `process` commands with multiple nodes to golang CLI as part of #17522 `bin/alluxio-start.sh masters` -> `bin/cli.sh process start masters` `bin/alluxio-start.sh job_masters` -> `bin/cli.sh process start job_masters` `bin/alluxio-start.sh workers` -> `bin/cli.sh process start workers` `bin/alluxio-start.sh job_workers` -> `bin/cli.sh process start job_workers` `bin/alluxio-start.sh proxies` -> `bin/cli.sh process start proxies` `bin/alluxio-start.sh all` -> `bin/cli.sh process start all` `bin/alluxio-start.sh local` -> `bin/cli.sh process start local` `bin/alluxio-stop.sh masters` -> `bin/cli.sh process stop masters` `bin/alluxio-stop.sh job_masters` -> `bin/cli.sh process stop job_masters` `bin/alluxio-stop.sh workers` -> `bin/cli.sh process stop workers` `bin/alluxio-stop.sh job_workers` -> `bin/cli.sh process stop job_workers` `bin/alluxio-stop.sh proxies` -> `bin/cli.sh process stop proxies` `bin/alluxio-stop.sh all` -> `bin/cli.sh process stop all` `bin/alluxio-stop.sh local` -> `bin/cli.sh process stop local` For command `process start/stop` on multiple nodes, using crypto's `ssh` package to create an SSH session, connect to masters, workers or all nodes, then send according subcommand on single nodes to these nodes. pr-link: #17887 Add job commands to golang CLI as part of #17522 `bin/alluxio-bash job cancel id` -> `bin/alluxio job cancel --id` `bin/alluxio-bash job leader` -> `bin/alluxio job leader` `bin/alluxio-bash job ls` -> `bin/alluxio job list` `bin/alluxio-bash job getCmdStatus jobControlId` -> `bin/alluxio job cmdStatus --id` `bin/alluxio-bash job stat [-v] id` -> `bin/alluxio job jobStatus [-v] --id` `bin/alluxio-bash fs distributedCp src dst` -> `bin/alluxio job submit --type cp --src --dst` `bin/alluxio-bash fs distributedMv src dst` -> `bin/alluxio job submit --type mv --src --dst` `bin/alluxio-bash fs load path --submit` -> `bin/alluxio job load --path` pr-link: #17931 Clean up multiprocess code - refactor commands launching the same process on multiple hosts into a single struct - simplify the logic to ssh into different hosts with more straightforward handling of error channel - move names.go under its own package to avoid import cycles pr-link: #17947 Add an `info version` subcommand to golang CLI as part of #17522 `bin/alluxio-bash version` -> `bin/alluxio info version` pr-link: #17943 update calls to old bash scripts to refer to alluxio-bash Add `cache` commands to golang CLI as part of #17522 `bin/alluxio-bash formatWorker` -> `bin/cli cache format` `bin/alluxio-bash fs freeWorker` -> `bin/cli cache free --worker` `bin/alluxio-bash fs free` -> `bin/cli cache free [-f] --path` `bin/alluxio-bash fs persist` -> `bin/cli cache persist --path` `bin/alluxio-bash fs setTtl` -> `bin/cli cache ttl --set --duration --action` `bin/alluxio-bash fs unsetTtl` -> `bin/cli cache ttl --unset` pr-link: #17937 backcompat support for start/stop scripts skip mount args in start script backcompat remove unsupported cmds

Add quorum commands to CLI

f52be55

Xenorith added area-shell Alluxio Command Line Interface type-feature This issue is a feature request labels Jun 7, 2023

Xenorith requested review from Kai-Zhang, jenoudet and jingwenouyang June 7, 2023 20:14

Xenorith commented Jun 7, 2023

View reviewed changes

jenoudet suggested changes Jun 7, 2023

View reviewed changes

refactor quorum cmds to parse --domain

b84dc47

jenoudet reviewed Jun 7, 2023

View reviewed changes

Xenorith commented Jun 7, 2023

View reviewed changes

Xenorith mentioned this pull request Jun 7, 2023

Alluxio CLI and start/stop scripts in golang #17522

Closed

10 tasks

jenoudet approved these changes Jun 7, 2023

View reviewed changes

alluxio-bot merged commit 4193e33 into Alluxio:golangCli Jun 8, 2023

Xenorith mentioned this pull request Jul 27, 2023

Add exec subcommand to CLI #17797

Merged

Xenorith added a commit that referenced this pull request Aug 3, 2023

Add quorum commands to CLI

88e7089

Add quorum/HA related commands as part of #17522 pr-link: #17570 change-id: cid-e9c76998b8fbfd023e0898c876cc495801a87fc5

Xenorith added a commit that referenced this pull request Aug 11, 2023

Add quorum commands to CLI

cd57234

Add quorum/HA related commands as part of #17522 pr-link: #17570 change-id: cid-e9c76998b8fbfd023e0898c876cc495801a87fc5

twalluxio pushed a commit to twalluxio/alluxio that referenced this pull request Aug 14, 2023

Add quorum commands to CLI

a095003

Add quorum/HA related commands as part of Alluxio#17522 pr-link: Alluxio#17570 change-id: cid-e9c76998b8fbfd023e0898c876cc495801a87fc5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quorum commands to CLI#17570

Add quorum commands to CLI#17570
alluxio-bot merged 2 commits intoAlluxio:golangClifrom
Xenorith:golangCliQuorum

Xenorith commented Jun 7, 2023

Xenorith Jun 7, 2023

jenoudet Jun 7, 2023

Xenorith Jun 7, 2023

jenoudet Jun 7, 2023

Xenorith Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet left a comment

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

jenoudet Jun 7, 2023

Xenorith left a comment

Xenorith commented Jun 8, 2023

maobaolong commented Jun 12, 2023

Xenorith commented Jun 13, 2023

Labels

4 participants

	domain string
	domain string `validate:"oneof=MASTER JOB_MASTER"`

	serverAddress string
	serverAddress string `validate:"hostname_port"`

Conversation

Xenorith commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenoudet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xenorith left a comment

Choose a reason for hiding this comment

Xenorith commented Jun 8, 2023

maobaolong commented Jun 12, 2023

Xenorith commented Jun 13, 2023

Labels

4 participants