101: Don't retry POSTs #120
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #101
What has been done to verify that this works as intended?
Ran the existing client tests to check that normal / low intensity usage is unaffected.
For the methods change, thought about adding more tests but they perhaps weren't very useful except to document the requirement that POSTs shouldn't be retried - like a test that does a POST and verifies that it's not retried? Probably if someone were considering changing the method list in future they would do some research and find this PR and the linked ticket.
For the timeout change, this might be useful to test i.e. to show how pyodk behaves when Central is taking a while to respond. If seems like a good idea I could do that, but I would suggest creating a dedicated performance test host rather than slamming dev/staging. Or add a local testing Central instance (e.g. with compose) so pyodk doesn't have an external dependency.
Why is this the best possible solution? Were any other approaches considered?
As discussed in the ticket and above it should reduce the likelihood of duplicate data if Central is taking a while to complete requests. An alternative up til now (and still) noted in the readme is to customise classes in
session.pyand provide a customSessionto theClient.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The worst case total wait time is 8.5min up from 2.5min which might be an issue if users of pyodk assumed a shorter timeout e.g. a job/script gets cancelled if it takes longer than 5 mins but they assumed it would try at least 2 things. Similarly users might have relied on POSTs being retried. But as mentioned above the old behaviour can be maintained by customising the
Session.Do we need any specific form for testing your changes? If so, please attach one.
N/A
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No this level of implementation detail isn't in the docs.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyodk testsandruff check pyodk teststo lint code