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

CLI Retry Options due to Intermittent Network #325

Closed
wants to merge 1 commit into from

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Feb 7, 2022

Description

This introduces retry options to CommandFind and CommandPing.

This was extracted out of #266 because it wasn't related to vaults functionality.

Still trying to find what the reason for these options are for, and perhaps this should be considered in light of the CLI over-all design.

Issues Fixed

Tasks

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

@joshuakarp do you have any comments about this? What would be the usecase of these retry options?

@CMCDragonkai
Copy link
Member Author

Remember, that sometimes retry can be done by the end user as well. They can do things like watch -n 1 command. Shell scripting can be used as well. But my question is what is the purpose of retrying for CommandFind and CommandPing?

Note that ping command itself repeatedly pings. Is the CommandPing meant to do this as well?

Also this PR currently has the incorrect short parameters in the tests. Always use the long parameters when testing or scripting CLI calls.

Base automatically changed from vaultspermissions to master March 9, 2022 09:12
@CMCDragonkai
Copy link
Member Author

This requires re-review due to big changes in our nodes ping and connection logic in #326.

@CMCDragonkai
Copy link
Member Author

So primarily the idea here is that one could do this for a few commands where it supports it. The --retry-count will retry something up to a limit, while --retry-interval will be the delay between retries.

At the same time, one must define what it means to retry something. What exceptions are considered retryable... maybe it depends on the command itself. It's not exactly generic.

At any case if this is needed, this needs to rebased and redone due to big changes in PK.

@CMCDragonkai
Copy link
Member Author

Closed because work is done on Polykey-CLI now.

@CMCDragonkai
Copy link
Member Author

New issue tracking this MatrixAI/Polykey-CLI#16

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

Successfully merging this pull request may close these issues.

1 participant