Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type in the exact number of machines to proceed? #454

Open
alexbozhenko opened this issue Nov 23, 2020 · 5 comments
Open

Type in the exact number of machines to proceed? #454

alexbozhenko opened this issue Nov 23, 2020 · 5 comments

Comments

@alexbozhenko
Copy link
Contributor

Hi.

I found idea in this blog post may be useful in clush: https://rachelbythebay.com/w/2020/10/26/num/

Would you be open to a pull request that adds a configurable confirmation like described in the post?

@alexbozhenko
Copy link
Contributor Author

@thiell what do you think?

@thiell
Copy link
Collaborator

thiell commented Dec 1, 2020

@alexbozhenko apologies for the delay. I would say, if you have the code ready, please submit the PR so we can have a look. If the patch is too big, we probably won't include it though, as we try to keep things as simple as possible, but in any case, it could be of interest to others. Thx!

@volans-
Copy link
Contributor

volans- commented Dec 1, 2020

For what is worth we've done it in Cumin (that uses ClusterShell underneath for the transport) and opted to avoid the thousands separator bit as it felt a bit too much. I think it would be a nice to have addition.

@degremont
Copy link
Collaborator

Additionally to what Stéphane said. I think you should be able to have a small patch for that, I would recommend:

  • Use a configuration option to enable this behavior. Should that be a node count limit? If the number of target is greater than this value, trigger the confirmation. Keeping this value unset disable the check. ClusterShell has code that helps creating config option that could be also use on command line easily.
  • Implement this in Clush code, after nodeset_base has been fully computed.
    That should be enough to do what you want.
    That should be completed with:
  • Doc update for the new config options
  • Tests

@alexbozhenko
Copy link
Contributor Author

For what is worth we've done it in Cumin (that uses ClusterShell underneath for the transport) and opted to avoid the thousands separator bit as it felt a bit too much. I think it would be a nice to have addition.

Oh, I see. wikimedia/cumin@5c18e5c
Thanks for the reference!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants