[GitHub] [hive] ashutosh-bapat commented on a change in pull request #669: HIVE-21864: LlapBaseInputFormat#closeAll() throws ConcurrentModificationException

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

ashutosh-bapat commented on a change in pull request #669: HIVE-21864: LlapBaseInputFormat#closeAll() throws ConcurrentModificationException
URL: https://github.com/apache/hive/pull/669#discussion_r292867947
 
 

 ##########
 File path: llap-ext-client/src/java/org/apache/hadoop/hive/llap/LlapBaseInputFormat.java
 ##########
 @@ -345,8 +346,11 @@ public static void close(String handleId) throws IOException {
    */
   public static void closeAll() {
     LOG.debug("Closing all handles");
-    for (String handleId : connectionMap.keySet()) {
+    Iterator<String> handleIds = connectionMap.keySet().iterator();
+    String handleId = null;
+    while (handleIds.hasNext()) {
       try {
+        handleId = handleIds.next();
 
 Review comment:
   Looking at https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html, esp. the following para
   
   > The iterators returned by all of this class's "collection view methods" are fail-fast: if the map is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException. Thus, in the face of concurrent modification, the iterator fails quickly and cleanly, rather than risking arbitrary, non-deterministic behavior at an undetermined time in the future.
   
   it looks like even the new code is prone to the same exception. Is the synchronize block in close() protects the code from exception?. Ideally we should be using iterator.remove() method to remove the element while iterating over keySet().
   
   I think, what we want to do here is
   1. Write two close() methods, one accepting a handleId and other accepting handleConnections. The first gets the handleConnections from handleId and calls the second.
   2. Modify closeAll() method to iterate over the connectionMap.keySet() using an interator. For every element in the interator, get the handleConnections and call close() method with those handleConnections. Now call remove() on the iterator.
   
   Does that look good?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org