michael-carter-instaclustr commented on a change in pull request #8844:
URL: https://github.com/apache/kafka/pull/8844#discussion_r443387368
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java
##########
@@ -56,7 +56,7 @@
private static final String THREAD_NAME_PREFIX = "task-thread-";
protected final ConnectorTaskId id;
- private final TaskStatus.Listener statusListener;
+ protected final TaskStatus.Listener statusListener;
Review comment:
Yes, I had the same thought and initially started going down that road.
The source of the problem as it appeared to me was that the failure methods
inside the WorkerConnector/WorkerTasks were catching any failure and not
propagating any exception back up to Worker where it would be able to record
metrics. Simply allowing the exception to filter back up seemed the way to go,
but since I needed to differentiate between a failure in startup and a failure
during regular execution, separating those methods seemed a good way to do it.
This worked very well for the connector at the time, but got a bit more
difficult for the tasks because they were being sent to an executor service, so
there wan’t an obvious exception handler in the Worker class to handle
problems. (I’ve noticed that since I looked at this, you’ve committed a change
that makes the connectors use an executor service too, so this probably now
applies to connectors as well as tasks). I wasn’t entirely certain whether it
was important or not that the startup code run on the same thread as the
regular execution, but assumed that it was, so started putting in a chain of
CompletableFutures where I could check for exceptions in the other thread and
only go on to submit the execute stage if the initialiseAndStart stage
completed successfully. But this required there to be two different entry
points for execution into the WorkerConnector/WorkerTasks which kind of
defeated the point of them implementing the Runnable interface, and the
exception checking was a bit ugly anyway. It was at this point that I
discovered the statusListener and thought that might be a cleaner way to go.
Not the only way of course, but it seemed to me to be a smaller change.
Separating those methods would be more easily achieved if the startup phase
could run on the same thread as the Worker, but that seems to me like more of a
significant change?
----------------------------------------------------------------
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
