-
Notifications
You must be signed in to change notification settings - Fork 68
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
[SYNSD-1233] Adding in a try/catch for HTTP 409 conflicts on entity creation #1131
base: develop
Are you sure you want to change the base?
Conversation
Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-27 19:12:34 UTC |
spy_tls_stream.spy_return | ||
) | ||
call_count = call_count + 1 | ||
# This is very brittle, however, I do not see data to determine what the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 Does that mean if we add functions called in the async stream that we would have edit this call_count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if more or less calls occur during the store_async
function we would need to adjust this call count.
This specific way of replacing the read isn't working on GH actions. Though I did confirm the logic is working on my local machine. This needs to be fixed, but more digging into this is needed. I can get the hostname/IP addresses used, but being this "far" down in the libraries and working directly in httpcore, I lose a lot of context like request headers and the uri.
Problem:
Solution:
ReadError
andReadTimeout
as exceptions that are retried. Instead let theSynapseHTTPError
be raised and wrap the POST call in a try/except that will get the entity from Synapse if it was a conflict, then compare the results to what the expected state was.Testing:
syncToSynapse
command and I manually triggered the exception to be raised.