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

Use ConcurrentHashMap to protect FS_CACHE #480

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

Conversation

hanneskaeufler
Copy link

Closes #474

*Issue #, if available: #474

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@markjschreiber
Copy link
Contributor

The checks are passing now and the code looks good. Do you want to remove the draft status?

@hanneskaeufler hanneskaeufler marked this pull request as ready for review June 24, 2024 19:32
@hanneskaeufler
Copy link
Author

The checks are passing now and the code looks good. Do you want to remove the draft status?

Sure! To be transparent though: I have not verified whatsoever that this solves the problem I was seeing in the referenced issue. It was more just a hunch that crossed my mind and I wanted to get to paper. I see no particular risk with this either, so we might as well get it in? Not sure about test automation here ... :/

@markjschreiber
Copy link
Contributor

The checks are passing now and the code looks good. Do you want to remove the draft status?

Sure! To be transparent though: I have not verified whatsoever that this solves the problem I was seeing in the referenced issue. It was more just a hunch that crossed my mind and I wanted to get to paper. I see no particular risk with this either, so we might as well get it in? Not sure about test automation here ... :/

Agreed, there is no risk. There might be a slight performance drop but should be trivial compared to the network IO we have to do with S3. I don't know that you could unit test this other than to ensure that the Map is an instance of ConcurrentMap just to make sure nobody changes it back again without good reason.

I assume you would want to test it with your parallel test framework and it is always a good idea to try running the apps in the examples dir to ensure nothing weird happens.

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.

java.util.ConcurrentModificationException after upgrading to 2.0.1 from 1.2.4
2 participants