You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/06 09:43:22 UTC
[GitHub] [druid] gianm opened a new pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
gianm opened a new pull request #10247:
URL: https://github.com/apache/druid/pull/10247
These other methods include:
1) New method CloseableUtils.closeAndWrapExceptions, which wraps IOExceptions
in RuntimeExceptions for callers that just want to avoid dealing with
checked exceptions. Most usages were migrated to this method, because it
looks like they were mainly attempts to avoid declaring a throws clause,
and perhaps were unintentionally suppressing IOExceptions.
2) New method CloseableUtils.closeInCatch, designed to properly close something
in a catch block without losing exceptions. Some usages from catch blocks
were migrated here, when it seemed that they were intended to avoid checked
exception handling, and did not really intend to also suppress IOExceptions.
3) New method CloseableUtils.closeAndSuppressExceptions, which sends all
exceptions to a "chomper" that consumes them. Nothing is thrown or returned.
The behavior is slightly different: with this method, _all_ exceptions are
suppressed, not just IOExceptions. Calls that seemed like they had good
reason to suppress exceptions were migrated here.
4) Some calls were migrated to try-with-resources, in cases where it appeared
that CloseQuietly was being used to avoid throwing an exception in a finally
block.
🎵 You don't have to go home, but you can't stay here... 🎵
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-751252639
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] closed pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #10247:
URL: https://github.com/apache/druid/pull/10247
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553771533
##########
File path: processing/src/main/java/org/apache/druid/segment/QueryableIndexIndexableAdapter.java
##########
@@ -280,7 +281,7 @@ public boolean hasTimeAndDimsChangedSinceMark()
@Override
public void close()
{
- CloseQuietly.close(closer);
+ CloseableUtils.closeAndWrapExceptions(closer);
Review comment:
The superclass won't allow it. (It overrides `close` and declares it to throw nothing.) I guess I could change the superclass, but I didn't want to go that far.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-756907541
Fixed up some merge conflicts.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-797855009
Fixed up some more merge conflicts.
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-756506233
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553768204
##########
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:
I'll change it to `CloseableUtils.closeAndSuppressExceptions`
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553772142
##########
File path: processing/src/main/java/org/apache/druid/segment/data/BlockLayoutColumnarDoublesSupplier.java
##########
@@ -168,7 +168,7 @@ public void get(final double[] out, final int[] indexes, final int length)
protected void loadBuffer(int bufferNum)
{
- CloseQuietly.close(holder);
+ CloseableUtils.closeAndWrapExceptions(holder);
Review comment:
I didn't realize it didn't throw `IOException`. So actually, I could just remove the CloseableUtils call completely here, and replace it with `holder.close()`. I'll do that.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553771195
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -329,9 +329,8 @@ public boolean doMergeResults(final GroupByQuery query)
finalResultSupplier
);
}
- catch (Exception ex) {
- CloseQuietly.close(resultSupplier);
- throw ex;
+ catch (Throwable e) {
Review comment:
Now that I think about it more, I guess the reason is that we always want to close stuff, even if some weird Throwable got thrown. It's making it behave like a finally or like a try-with-resources. I changed it back to Throwable. Let me know if you disagree.
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis merged pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10247:
URL: https://github.com/apache/druid/pull/10247
--
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: commits-unsubscribe@druid.apache.org
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
[GitHub] [druid] gianm commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-949931168
Fixed up some more merge conflicts.
--
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: commits-unsubscribe@druid.apache.org
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553768704
##########
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:
Good point, changed this to suppress.
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-722085412
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553768375
##########
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:
I put them both in a single `CloseableUtils.closeAll`
##########
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 solution
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553770934
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -329,9 +329,8 @@ public boolean doMergeResults(final GroupByQuery query)
finalResultSupplier
);
}
- catch (Exception ex) {
- CloseQuietly.close(resultSupplier);
- throw ex;
+ catch (Throwable e) {
Review comment:
Hmm, I can't think of a reason. Changed it back to Exception.
----------------------------------------------------------------
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
[GitHub] [druid] jihoonson commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r482719520
##########
File path: core/src/test/java/org/apache/druid/utils/CloseableUtilsTest.java
##########
@@ -0,0 +1,415 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.utils;
+
+import com.google.common.base.Throwables;
+import org.hamcrest.CoreMatchers;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.internal.matchers.ThrowableCauseMatcher;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class CloseableUtilsTest
+{
+ private final TestCloseable quietCloseable = new TestCloseable(null);
+ private final TestCloseable quietCloseable2 = new TestCloseable(null);
+ private final TestCloseable ioExceptionCloseable = new TestCloseable(new IOException());
+ private final TestCloseable runtimeExceptionCloseable = new TestCloseable(new IllegalArgumentException());
+
+ // For closeAndSuppressException tests.
+ private final AtomicLong chomped = new AtomicLong();
+ private final Consumer<Exception> chomper = e -> chomped.incrementAndGet();
+
+ @Test
+ public void test_closeAll_array_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(quietCloseable, null, quietCloseable2);
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_list_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(Arrays.asList(quietCloseable, null, quietCloseable2));
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_array_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(quietCloseable, null, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
Review comment:
Hmm, why not `IllegalArgumentException`?
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -329,9 +329,8 @@ public boolean doMergeResults(final GroupByQuery query)
finalResultSupplier
);
}
- catch (Exception ex) {
- CloseQuietly.close(resultSupplier);
- throw ex;
+ catch (Throwable e) {
Review comment:
Why catching `Throwable`s vs catching only `Exception`s? It doesn't seem useful to do anything when you see any `Error` here.
##########
File path: processing/src/main/java/org/apache/druid/segment/data/BlockLayoutColumnarDoublesSupplier.java
##########
@@ -168,7 +168,7 @@ public void get(final double[] out, final int[] indexes, final int length)
protected void loadBuffer(int bufferNum)
{
- CloseQuietly.close(holder);
+ CloseableUtils.closeAndWrapExceptions(holder);
Review comment:
This seems fine because `ResourceHolder.close()` doesn't throw `IOException`, but this code seems confusing to me since, until looking at the code closely, I wasn't sure if it's fine to not execute the below lines when this line throws an exception. How about adding a comment here, such as "this line should never throw any checked exceptions"?
##########
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:
I agree that it would be better to suppress IOException. We shouldn't throw IOException immediately especially at [Line 650](https://github.com/apache/druid/pull/10247/files#diff-84792f9d3cefe47cbb471669dce2a276R650-R651) since the terminal resultBatch must be added to the outputQueue after closing all cursors.
##########
File path: processing/src/main/java/org/apache/druid/segment/QueryableIndexIndexableAdapter.java
##########
@@ -280,7 +281,7 @@ public boolean hasTimeAndDimsChangedSinceMark()
@Override
public void close()
{
- CloseQuietly.close(closer);
+ CloseableUtils.closeAndWrapExceptions(closer);
Review comment:
nit: you can modify the signature of this method to throw IOExceptoin, but this should also be fine.
##########
File path: core/src/test/java/org/apache/druid/utils/CloseableUtilsTest.java
##########
@@ -0,0 +1,415 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.utils;
+
+import com.google.common.base.Throwables;
+import org.hamcrest.CoreMatchers;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.internal.matchers.ThrowableCauseMatcher;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class CloseableUtilsTest
+{
+ private final TestCloseable quietCloseable = new TestCloseable(null);
+ private final TestCloseable quietCloseable2 = new TestCloseable(null);
+ private final TestCloseable ioExceptionCloseable = new TestCloseable(new IOException());
+ private final TestCloseable runtimeExceptionCloseable = new TestCloseable(new IllegalArgumentException());
+
+ // For closeAndSuppressException tests.
+ private final AtomicLong chomped = new AtomicLong();
+ private final Consumer<Exception> chomper = e -> chomped.incrementAndGet();
+
+ @Test
+ public void test_closeAll_array_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(quietCloseable, null, quietCloseable2);
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_list_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(Arrays.asList(quietCloseable, null, quietCloseable2));
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_array_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(quietCloseable, null, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
+ }
+
+ @Test
+ public void test_closeAll_list_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(
+ Arrays.asList(
+ quietCloseable,
+ null,
+ ioExceptionCloseable,
+ quietCloseable2,
+ runtimeExceptionCloseable
+ )
+ );
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
Review comment:
Same here.
##########
File path: processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
##########
@@ -530,13 +530,13 @@ public void close()
headerOut.writeInt(Ints.checkedCast(valuesOut.size()));
if (prevVal instanceof Closeable) {
- CloseQuietly.close((Closeable) prevVal);
+ CloseableUtils.closeAndWrapExceptions((Closeable) prevVal);
Review comment:
This changes the behavior, but any callers of `fromIterableVersionOne()` doesn't seem creating `GenericIndexed` from `Closeable` types. How about removing this?
##########
File path: processing/src/main/java/org/apache/druid/segment/data/BlockLayoutColumnarDoublesSupplier.java
##########
@@ -168,7 +168,7 @@ public void get(final double[] out, final int[] indexes, final int length)
protected void loadBuffer(int bufferNum)
{
- CloseQuietly.close(holder);
+ CloseableUtils.closeAndWrapExceptions(holder);
Review comment:
Same comment for other places where close `ResourceHolder`s.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553770448
##########
File path: core/src/test/java/org/apache/druid/utils/CloseableUtilsTest.java
##########
@@ -0,0 +1,415 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.utils;
+
+import com.google.common.base.Throwables;
+import org.hamcrest.CoreMatchers;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.internal.matchers.ThrowableCauseMatcher;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class CloseableUtilsTest
+{
+ private final TestCloseable quietCloseable = new TestCloseable(null);
+ private final TestCloseable quietCloseable2 = new TestCloseable(null);
+ private final TestCloseable ioExceptionCloseable = new TestCloseable(new IOException());
+ private final TestCloseable runtimeExceptionCloseable = new TestCloseable(new IllegalArgumentException());
+
+ // For closeAndSuppressException tests.
+ private final AtomicLong chomped = new AtomicLong();
+ private final Consumer<Exception> chomper = e -> chomped.incrementAndGet();
+
+ @Test
+ public void test_closeAll_array_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(quietCloseable, null, quietCloseable2);
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_list_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(Arrays.asList(quietCloseable, null, quietCloseable2));
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_array_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(quietCloseable, null, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
Review comment:
Ah, yeah, that's a better test. I changed it.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-756506222
nice try, stalebot
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r595598037
##########
File path: core/src/main/java/org/apache/druid/utils/CloseableUtils.java
##########
@@ -34,15 +42,129 @@
* first.close();
* second.close();
*
- * to have safety of {@link org.apache.druid.java.util.common.io.Closer}, but without associated boilerplate code
+ * to have safety of {@link Closer}, but without associated boilerplate code
* of creating a Closer and registering objects in it.
*/
- public static void closeBoth(Closeable first, Closeable second) throws IOException
+ public static void closeAll(Closeable first, Closeable... others) throws IOException
+ {
+ final List<Closeable> closeables = new ArrayList<>(others.length + 1);
+ closeables.add(first);
+ closeables.addAll(Arrays.asList(others));
+ closeAll(closeables);
+ }
+
+ /**
+ * Close all the provided {@param closeables}, from first to last.
+ */
+ public static <T extends Closeable> void closeAll(Iterable<T> closeables) throws IOException
+ {
+ final Closer closer = Closer.create();
+
+ // Register in reverse order, so we close from first to last.
+ closer.registerAll(Lists.reverse(Lists.newArrayList(closeables)));
+ closer.close();
+ }
+
+ /**
+ * Like {@link Closeable#close()}, but guaranteed to throw {@param caught}. Will add any exceptions encountered
+ * during closing to {@param caught} using {@link Throwable#addSuppressed(Throwable)}.
+ *
+ * Should be used like {@code throw CloseableUtils.closeInCatch(e, closeable)}. (The "throw" is important for
+ * reachability detection.)
+ */
+ public static <E extends Throwable> RuntimeException closeInCatch(
+ final E caught,
+ @Nullable final Closeable closeable
+ ) throws E
+ {
+ if (caught == null) {
+ // Incorrect usage; throw an exception with an error message that may be useful to the programmer.
+ final RuntimeException e1 = new IllegalStateException("Must be called with non-null caught exception");
+
+ if (closeable != null) {
+ try {
+ closeable.close();
+ }
+ catch (Throwable e2) {
+ e1.addSuppressed(e2);
+ }
+ }
+
+ throw e1;
+ }
+
+ if (closeable != null) {
+ try {
+ closeable.close();
+ }
+ catch (Throwable e) {
+ caught.addSuppressed(e);
+ }
+ }
+
+ throw caught;
+ }
+
+ /**
+ * Like {@link #closeInCatch} but wraps {@param caught} in a {@link RuntimeException} if it is a checked exception.
+ */
+ public static <E extends Throwable> RuntimeException closeAndWrapInCatch(
+ final E caught,
+ @Nullable final Closeable closeable
+ )
+ {
+ try {
+ throw closeInCatch(caught, closeable);
+ }
+ catch (RuntimeException | Error e) {
+ // Unchecked exception.
+ throw e;
+ }
+ catch (Throwable e) {
+ // Checked exception; must wrap.
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Like {@link Closeable#close()} but wraps IOExceptions in RuntimeExceptions.
+ */
+ public static void closeAndWrapExceptions(@Nullable final Closeable closeable)
{
- //noinspection EmptyTryBlock
- try (Closeable ignore1 = second;
- Closeable ignore2 = first) {
- // piggy-back try-with-resources semantics
+ if (closeable == null) {
+ return;
+ }
+
+ try {
+ closeable.close();
+ }
+ catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Like {@link Closeable#close()} but sends any exceptions to the provided Consumer, and then throws them away.
+ *
+ * If the Consumer throws an exception, that exception is thrown by this method. So if your intent is to chomp
+ * exceptions, you should avoid writing a Consumer that might thrown an exception.
Review comment:
```suggestion
* exceptions, you should avoid writing a Consumer that might throw an exception.
```
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553770529
##########
File path: core/src/test/java/org/apache/druid/utils/CloseableUtilsTest.java
##########
@@ -0,0 +1,415 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.utils;
+
+import com.google.common.base.Throwables;
+import org.hamcrest.CoreMatchers;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.internal.matchers.ThrowableCauseMatcher;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class CloseableUtilsTest
+{
+ private final TestCloseable quietCloseable = new TestCloseable(null);
+ private final TestCloseable quietCloseable2 = new TestCloseable(null);
+ private final TestCloseable ioExceptionCloseable = new TestCloseable(new IOException());
+ private final TestCloseable runtimeExceptionCloseable = new TestCloseable(new IllegalArgumentException());
+
+ // For closeAndSuppressException tests.
+ private final AtomicLong chomped = new AtomicLong();
+ private final Consumer<Exception> chomper = e -> chomped.incrementAndGet();
+
+ @Test
+ public void test_closeAll_array_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(quietCloseable, null, quietCloseable2);
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_list_quiet() throws IOException
+ {
+ CloseableUtils.closeAll(Arrays.asList(quietCloseable, null, quietCloseable2));
+ assertClosed(quietCloseable, quietCloseable2);
+ }
+
+ @Test
+ public void test_closeAll_array_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(quietCloseable, null, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
+ }
+
+ @Test
+ public void test_closeAll_list_loud()
+ {
+ Exception e = null;
+ try {
+ CloseableUtils.closeAll(
+ Arrays.asList(
+ quietCloseable,
+ null,
+ ioExceptionCloseable,
+ quietCloseable2,
+ runtimeExceptionCloseable
+ )
+ );
+ }
+ catch (Exception e2) {
+ e = e2;
+ }
+
+ assertClosed(quietCloseable, ioExceptionCloseable, quietCloseable2, runtimeExceptionCloseable);
+
+ // First exception
+ Assert.assertThat(e, CoreMatchers.instanceOf(IOException.class));
+
+ // Second exception
+ Assert.assertEquals(1, e.getSuppressed().length);
+ Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));
Review comment:
Also changed this.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553772787
##########
File path: processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
##########
@@ -530,13 +530,13 @@ public void close()
headerOut.writeInt(Ints.checkedCast(valuesOut.size()));
if (prevVal instanceof Closeable) {
- CloseQuietly.close((Closeable) prevVal);
+ CloseableUtils.closeAndWrapExceptions((Closeable) prevVal);
Review comment:
If it's ok with you, I'd prefer to leave it in for this patch, because I didn't want to go through and make sure it's always ok to skip closing.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10247:
URL: https://github.com/apache/druid/pull/10247#discussion_r553772428
##########
File path: processing/src/main/java/org/apache/druid/segment/data/BlockLayoutColumnarDoublesSupplier.java
##########
@@ -168,7 +168,7 @@ public void get(final double[] out, final int[] indexes, final int length)
protected void loadBuffer(int bufferNum)
{
- CloseQuietly.close(holder);
+ CloseableUtils.closeAndWrapExceptions(holder);
Review comment:
Did that, along with the other block layout column suppliers.
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by 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
[GitHub] [druid] gianm commented on pull request #10247: Remove CloseQuietly and migrate its usages to other methods.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10247:
URL: https://github.com/apache/druid/pull/10247#issuecomment-756907541
Fixed up some merge conflicts.
----------------------------------------------------------------
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