fix: make CloudStorageFileSystem#forBucket thread safe #719
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(cloudStorageConfiguration, storageOptions); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Thanks for coming up with a fix so quickly! Just one comment about overriding the |
There was a problem hiding this comment.
Thanks @BenWhitehead! LGTM, added a few request for changes. This follows what we discussed earlier this week.
...o/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java
Outdated
Show resolved
Hide resolved
google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java
Outdated
Show resolved
Hide resolved
google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java
Outdated
Show resolved
Hide resolved
mistakenly included test in Provider test even though it doesn't do anything with the provider api
Replace manual attempt at caching via a HashMap with a guava Cache.
Fix #691
Fix #698