C0urante commented on code in PR #11779:
URL: https://github.com/apache/kafka/pull/11779#discussion_r891910326
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java:
##########
@@ -836,7 +883,9 @@ public void deleteConnectorConfig(final String connName,
final Callback<Created<
callback.onCompletion(new NotFoundException("Connector " +
connName + " not found"), null);
} else {
log.trace("Removing connector config {} {}", connName,
configState.connectors());
- configBackingStore.removeConnectorConfig(connName);
+ if (!writeToConfigTopicAsLeader(() ->
configBackingStore.removeConnectorConfig(connName))) {
+ throw new ConnectException("Failed to remove connector
configuration from config topic since worker was fenced out");
+ }
callback.onCompletion(null, new Created<>(false, null));
Review Comment:
On second thought, I think it's probably fine to leave things as they are
without adding a manual invocation of `Callback::onCompletion` and a `return
null`. Yes, `writeToConfigTopicAsLeader` may throw an exception, but so could
writes to the config topic before changes for this KIP were made (such as
[here](https://github.com/apache/kafka/blob/8e205b503a39ba8c798e3ce14cd887b66a88551c/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L835),
[here](https://github.com/apache/kafka/blob/8e205b503a39ba8c798e3ce14cd887b66a88551c/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L894),
and
[here](https://github.com/apache/kafka/blob/8e205b503a39ba8c798e3ce14cd887b66a88551c/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L993).
If we were throwing an exception from within the body of the herder request
instead of a method that the request invokes, it'd make sense to change that to
instead be a manual invocation of the callback with the exception. But just
calling a method that might throw an exception is different, and follows
existing precedent in the code base without having to jump through special
callback-related hoops.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
