kkonstantine commented on a change in pull request #8618:
URL: https://github.com/apache/kafka/pull/8618#discussion_r421005156
##########
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:
Let me see if I understand what you are describing as `wrapped`.
My use case is as follows:
`SinkTask#close` attempts to release resources and if it fails it throws a
`ConnectException` as we'd expect from connector developers to do (currently it
throws a `RuntimeException` which might be less representative).
With your fix this exception can appear as suppressed when an exception
happens in `SinkTask#put` and that's what your test is guarding against.
My point is to add the missing test case for when the exception on `close`
is the only exception that is thrown. There is a variety of ways to do that,
but I agree with you, this test is not there now. However, I don't think this
necessarily makes it out of scope.
----------------------------------------------------------------
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:
us...@infra.apache.org
