[GitHub] [druid] clintropolis commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.

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

clintropolis commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r480621837



##########
File path: processing/src/test/java/org/apache/druid/segment/data/V3CompressedVSizeColumnarMultiIntsSerializerTest.java
##########
@@ -395,7 +395,7 @@ private void generateV2SerializedSizeAndData(long numRows, int maxValue, int max
           Assert.assertEquals(subVals.get(j), expected[j]);
         }
       }
-      CloseQuietly.close(columnarMultiInts);
+      CloseableUtils.closeAndWrapExceptions(columnarMultiInts);

Review comment:
       same comment about closing mapper

##########
File path: processing/src/main/java/org/apache/druid/segment/join/HashJoinSegment.java
##########
@@ -122,15 +122,14 @@ public void close() throws IOException
         }).orElse(true);
       }
       if (acquireFailed) {
-        CloseQuietly.close(closer);
+        CloseableUtils.closeAndWrapExceptions(closer);
         return Optional.empty();
       } else {
         return Optional.of(closer);
       }
     }
-    catch (Exception ex) {
-      CloseQuietly.close(closer);
-      return Optional.empty();

Review comment:
       This method was using `CloseQuietly` to eat exceptions, although probably not well enough since it was only eating `IOExceptions`. I think it should probably be using `closeAndSuppressExceptions`. 
   
   Per the javadocs of `acquireReferences`:
   
   ```
   ...
      * IMPORTANT NOTE: to fulfill the contract of this method, implementors must return a closeable to indicate that the
      * reference can be acquired, even if there is nothing to close. Implementors should avoid allowing this method or the
      * {@link Closeable} it creates to throw exceptions.
      *
      * For callers: if this method returns non-empty, IT MUST BE CLOSED, else reference counts can potentially leak.
   ...
   ```

##########
File path: core/src/main/java/org/apache/druid/java/util/common/guava/ParallelMergeCombiningSequence.java
##########
@@ -1376,6 +1382,6 @@ long getTotalCpuTimeNanos()
   {
     Closer closer = Closer.create();
     closer.registerAll(cursors);
-    CloseQuietly.close(closer);
+    CloseableUtils.closeAndWrapExceptions(closer);

Review comment:
       Looking at the usage of this method, it appears that it is expected to eat the exceptions:
   
   ```
         catch (Throwable t) {
           closeAllCursors(sequenceCursors);
           cancellationGizmo.cancel(t);
           out.offer(ResultBatch.TERMINAL);
         }
   ```
   so, I think this method should be using `closeAndSuppressExceptions`. I guess callers could also be re-arranged to do the things that absolutely need done before closing the cursors, however, nothing would directly catch an exception thrown here, so it is sort of like screaming into the void, so I think we should just suppress them. 

##########
File path: processing/src/test/java/org/apache/druid/segment/data/V3CompressedVSizeColumnarMultiIntsSerializerTest.java
##########
@@ -322,7 +322,7 @@ private void checkV2SerializedSizeAndData(int offsetChunkFactor, int valueChunkF
           Assert.assertEquals(subVals.get(j), vals.get(i)[j]);
         }
       }
-      CloseQuietly.close(columnarMultiInts);
+      CloseableUtils.closeAndWrapExceptions(columnarMultiInts);

Review comment:
       Hmm, I know this is a test and probably doesn't really matter, but it seems like `mapper.close` should get called probably. Should this be using `closeAndSuppressExceptions` to ensure that it gets closed, or make them both part of a closer that gets passed into this method?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org