kkonstantine commented on a change in pull request #8618:
URL: https://github.com/apache/kafka/pull/8618#discussion_r420895742
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSinkTask.java
##########
@@ -193,13 +194,11 @@ public void transitionTo(TargetState state) {
@Override
public void execute() {
initializeAndStart();
- try {
+ // Make sure any uncommitted data has been committed and the task has
+ // a chance to clean up its state
+ try (QuietClosable ignored = this::closePartitions) {
Review comment:
Again naming here can be misleading. `ignored` is more like unused in
the try block.
But also that's not the point of this idiom. It's about suppressing
exceptions from `finally` instead of the originator.
How about `suppressible`? Also, `unused` might be even better, because
`ignored` is untrue especially if `closePartitions` is the only method that
throws. But I think `suppressible` highlights the intentions here specifically.
##########
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##########
@@ -885,6 +885,18 @@ public static void closeAll(Closeable... closeables)
throws IOException {
throw exception;
}
+ /**
+ * An {@link AutoCloseable} interface without a throws clause in the
signature
+ *
+ * This is used with lambda expressions in try-with-resources clauses
+ * to avoid casting un-checked exceptions to checked exceptions
unnecessarily.
+ */
+ @FunctionalInterface
+ public interface QuietClosable extends AutoCloseable {
Review comment:
```suggestion
public interface QuietClosable extends AutoCloseable {
```
I don't think the name is very accurate. The interface does not prevent
implementation of close from throwing (as opposed to the methods below) and its
demonstrated use does not either. (there's also a typo, if typos in non-words
matter).
How about `UncheckedCloseable`?
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSinkTaskTest.java
##########
@@ -856,6 +858,47 @@ public void run() {
PowerMock.verifyAll();
}
+ @Test
+ public void testSinkTasksHandleCloseErrors() throws Exception {
Review comment:
Can you also write a test where an exception is thrown only by
`sinkTask.close`? Actually, we could keep the name for this new test here _as
is_, and the new test could be named in a way that tells us that the exceptions
on close are suppressed in the presence of exceptions in the main try block.
----------------------------------------------------------------
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]
