Skip to content

[Caching] Remove the error for creating multiple CAS at same location #1586

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

Merged

Conversation

cachemeifyoucan
Copy link
Contributor

Remove the CASError that prevents creating two different CAS at the same CAS path. This allows build system to create compatible CAS at the same location even the configuration is slightly different.

Remove the CASError that prevents creating two different CAS at the same
CAS path. This allows build system to create compatible CAS at the same
location even the configuration is slightly different.
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

My concern with this change is that it will make it harder to diagnose real bugs where you have mismatched configurations. It does seems reasonable for the plugin options to differ here, because we don't have any way to interpret them, but maybe we should keep the error if the plugin path differs?

@cachemeifyoucan
Copy link
Contributor Author

My concern with this change is that it will make it harder to diagnose real bugs where you have mismatched configurations

While this is true, the benefit is quite limited as it only catches the case where it is created inside the same Oracle. You can still create with different oracle and slip through.

Also the main reason for this error is because the default OnDiskCAS implementation can race with in the process if you have multiple instances of same CAS. This error should be fixed (hopefully) and no longer a problem.

@akyrtzi
Copy link
Contributor

akyrtzi commented Apr 25, 2024

My concern with this change is that it will make it harder to diagnose real bugs where you have mismatched configurations

While this is true, the benefit is quite limited as it only catches the case where it is created inside the same Oracle. You can still create with different oracle and slip through.

Also the main reason for this error is because the default OnDiskCAS implementation can race with in the process if you have multiple instances of same CAS. This error should be fixed (hopefully) and no longer a problem.

I agree, I think this API is the wrong level for a "mismatched config" check, the caller should be allowed to create the CAS instances it wants and use its own sanity checks.

@cachemeifyoucan cachemeifyoucan merged commit 2ed37d1 into swiftlang:main Apr 25, 2024
3 checks passed
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.

3 participants