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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
