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

fix: make CloudStorageFileSystem#forBucket thread safe #719

Merged
merged 6 commits into from Sep 27, 2021
Merged

Conversation

BenWhitehead
Copy link
Contributor

@BenWhitehead BenWhitehead commented Sep 23, 2021

Replace manual attempt at caching via a HashMap with a guava Cache.

Fix #691
Fix #698

Replace manual attempt at caching via a HashMap with a guava Cache.

Fix #691
Fix #698
@BenWhitehead BenWhitehead requested a review from as a code owner Sep 23, 2021
@product-auto-label product-auto-label bot added the api: storage label Sep 23, 2021
@google-cla google-cla bot added the cla: yes label Sep 23, 2021
@BenWhitehead BenWhitehead requested a review from as a code owner Sep 23, 2021
Copy link

@tritone tritone left a comment

Should probably get a java person to approve this as well, but... really nice!

@Override
public int hashCode() {
return Objects.hash(cloudStorageConfiguration, storageOptions);
}

Choose a reason for hiding this comment

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

These overrides of equals and hashCode remind me of Brian's comment (2) in #691 (comment) - is this now ok here because we're no longer looking up via HashMap (in which case is the override still necessary?) Or are we now waiting until after initialization before caching? Or is Brian's comment still potentially a problem?

Copy link
Contributor Author

@BenWhitehead BenWhitehead Sep 24, 2021

Choose a reason for hiding this comment

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

The .equals from that comment was comparing instances of CloudStorageFileSystemProvider which aren't immutable. Here we're holding on to CloudStorageConfig which is immutable via AutoValue and a StorageOptions which has a stable hashCode & equals which don't include transient state. So in this case, we're only keying off the config none of the running state.

Choose a reason for hiding this comment

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

Great, thanks! Looks good to me!

@cjllanwarne
Copy link

@cjllanwarne cjllanwarne commented Sep 24, 2021

Thanks for coming up with a fix so quickly! Just one comment about overriding the hashCode and equals in ways that might case the values to change after the object is initialized, but otherwise this looks great!

Copy link
Member

@frankyn frankyn left a comment

Thanks @BenWhitehead! LGTM, added a few request for changes. This follows what we discussed earlier this week.

mistakenly included test in Provider test even though it doesn't do anything with the provider api
Copy link
Member

@frankyn frankyn left a comment

LGTM, 🚢

@BenWhitehead BenWhitehead merged commit ac8bfee into master Sep 27, 2021
16 checks passed
@BenWhitehead BenWhitehead deleted the fix-691 branch Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage cla: yes
Projects
None yet
5 participants