[GitHub] [kafka] kkonstantine commented on a change in pull request #8618: KAFKA-9955: Prevent SinkTask::close from shadowing other exceptions

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

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:
users@infra.apache.org