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

Draft: UUID regeneration on retry #26

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

wseaton
Copy link
Contributor

@wseaton wseaton commented Feb 25, 2024

This MR attempts to fix a bug where UUIDs are not properly regenerated on retries triggered by middleware. By moving request parameters for UUID and Timestamp generation into middleware, we fix that, but also allow future enhancements for more robust error handling modes like what is specified in: https://docs.snowflake.com/en/developer-guide/sql-api/submitting-requests#resubmitting-a-request-to-execute-sql-statements.

I still need to figure out a good solution for testing this, might end up mocking one of the endpoints.

We should be able to leverage Extension state to detect if a request is a retry, and based on the user's client configuration hint snowflake that it should "rescan" to see if the query was successful or not.

@wseaton wseaton changed the title UUID regeneration on retry Draft: UUID regeneration on retry Feb 26, 2024
@wseaton
Copy link
Contributor Author

wseaton commented Feb 26, 2024

@andrusha @dmzmk this is ready for review (no rush), I had to make some Connection API modifications to make the API client mockable. But the mock verifies the current middleware behavior, and builder methods could be added to SnowflakeApiBuilder to have it also use a mocked client which might be nice for adding tests for arrow serde or other things.

@andrusha
Copy link
Owner

The docs are for the json api, which is different from odbc api this lib implements, looking at the https://github.com/snowflakedb/gosnowflake/blob/2f775957a87540a93078051bc132991777ec715a/retry.go#L49 they do pass a similar parameter however, but it's retryCount instead, so it'll be useful

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

Successfully merging this pull request may close these issues.

2 participants