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

Reject new stage metadata if the deployment id does not match what the client was created with #794

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

sfc-gh-psaha
Copy link
Contributor

@sfc-gh-psaha sfc-gh-psaha commented Jul 15, 2024

This prevents issues when there is transparent failover using Client Redirect or another similar mechanism. Along the way, I found some missing/misconfigured error message resource and fixed them.

@sfc-gh-psaha sfc-gh-psaha marked this pull request as ready for review July 15, 2024 16:51
@sfc-gh-psaha sfc-gh-psaha requested review from sfc-gh-tzhang and a team as code owners July 15, 2024 16:51
Copy link
Collaborator

@sfc-gh-rcheng sfc-gh-rcheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks!

Copy link
Contributor

@sfc-gh-asen sfc-gh-asen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/main/java/net/snowflake/ingest/utils/ErrorCode.java Outdated Show resolved Hide resolved
0035=Failed to load {0}. If you use FIPS, import BouncyCastleFipsProvider in the application: {1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sfc-gh-alhuang FYI, let's make sure to update the error message as well when removing an error, or it might be a good idea to never reuse error codes as customer might have custom dependency on them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 never remove previously used error codes - maybe just rename them if needed to indicate that they are not used anymore.

@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-SNOW-1511536-match-deployment branch from c8c7a5a to 0ef0e29 Compare July 16, 2024 15:55
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-SNOW-1511536-match-deployment branch from 0ef0e29 to 1e9e738 Compare July 16, 2024 16:03
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-SNOW-1511536-match-deployment branch from 1e9e738 to ef07694 Compare July 16, 2024 16:09
0035=Failed to load {0}. If you use FIPS, import BouncyCastleFipsProvider in the application: {1}
0036=Failed to drop channel: {0}
0037=Deployment ID mismatch, Client was created on: {0}, Got upload location for: {1}. Please restart client: {2}.
Copy link
Collaborator

@sfc-gh-japatel sfc-gh-japatel Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be please recreate client? (as there is no restart functionality)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant it as "shut down your application and restart it".

Copy link
Collaborator

@sfc-gh-japatel sfc-gh-japatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@sfc-gh-psaha sfc-gh-psaha merged commit 7310921 into master Jul 16, 2024
48 checks passed
@sfc-gh-psaha sfc-gh-psaha deleted the psaha-SNOW-1511536-match-deployment branch July 16, 2024 18:16
sfc-gh-kgaputis pushed a commit to sfc-gh-kgaputis/snowflake-ingest-java that referenced this pull request Sep 12, 2024
…e client was created with (snowflakedb#794)

* Reject new stage metadata if the deployment id does not match what the client was created with

* Review comments
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.

5 participants