[GitHub] [beam] lukecwik commented on a diff in pull request #17201: [BEAM-14187] Fix concurrency issue in IsmReaderImpl

[GitBox <gi...@apache.org>]

lukecwik commented on code in PR #17201:
URL: https://github.com/apache/beam/pull/17201#discussion_r845437349


##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/IsmReaderImpl.java:
##########
@@ -370,7 +373,7 @@ boolean bloomFilterMightContain(RandomAccessData keyBytes) {
     position(rawChannel, footer.getBloomFilterPosition());
     bloomFilter = ScalableBloomFilterCoder.of().decode(Channels.newInputStream(rawChannel));
 
-    indexPerShard = new HashMap<>();
+    indexPerShard = new ConcurrentHashMap<>();

Review Comment:
   Note that initializeBloomFilterAndIndexPerShard is synchronized which is the only method that mutates the indexPerShard variable. Since initializeBloomFilterAndIndexPerShard is invoked within overKeyComponents that means there is a memory barrier between the thread that created the indexPerShard and other threads ensuring that the object is safely published across threads.
   
   Also, the [Javadoc for HashMap](https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html) says that concurrent reads are ok, its concurrent modifications that require synchronization:
   "If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally."



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org