C0urante commented on code in PR #12568:
URL: https://github.com/apache/kafka/pull/12568#discussion_r961796742
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java:
##########
@@ -294,7 +294,12 @@ public synchronized void restartConnector(String connName,
Callback<Void> cb) {
worker.stopAndAwaitConnector(connName);
- startConnector(connName, (error, result) -> cb.onCompletion(error,
null));
+ startConnector(connName, (error, targetState) -> {
+ if (targetState == TargetState.STARTED) {
+ requestTaskReconfiguration(connName);
Review Comment:
I think this is fine as-is, we synchronize in `requestTaskReconfiguration`
either way. I can't think of a significant difference between invoking that
method in the `WorkerConnector`'s thread and invoking it via the herder's
`requestExecutorService`.
In fact, I don't think we need to invoke anything via the
`requestExecutorService` except in the variant of `restartConnector` that
accepts a delay. But that's out of scope for this PR and should be addressed
separately in order to simplify review.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##########
@@ -812,7 +861,7 @@ public void testPutConnectorConfig() throws Exception {
FutureCallback<Herder.Created<ConnectorInfo>> reconfigureCallback =
new FutureCallback<>();
herder.putConnectorConfig(CONNECTOR_NAME, newConnConfig, true,
reconfigureCallback);
Herder.Created<ConnectorInfo> newConnectorInfo =
reconfigureCallback.get(1000L, TimeUnit.SECONDS);
- ConnectorInfo newConnInfo = new ConnectorInfo(CONNECTOR_NAME,
newConnConfig, Arrays.asList(new ConnectorTaskId(CONNECTOR_NAME, 0)),
+ ConnectorInfo newConnInfo = new ConnectorInfo(CONNECTOR_NAME,
newConnConfig, singletonList(new ConnectorTaskId(CONNECTOR_NAME, 0)),
Review Comment:
My IDE has been nagging me about this one too, for years. I've left it as-is
(maybe not here but in other places) since it's not performance-critical and
it's easier to modify in case we need to add other elements, or construct an
empty list. I think we should revert this 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
