You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/02/22 00:53:43 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13287: MINOR: Refactor task change logic to AbstractHerder, reuse for standalone mode.
gharris1727 opened a new pull request, #13287:
URL: https://github.com/apache/kafka/pull/13287
The condition to check for whether task configs has changed is duplicated in the Standalone and Distributed Herder classes.
This change refactors the implementation from DistributedHerder to cover the standalone, without changing the behavior.
Also alters the debug logs to include the connector name, and a log message to indicate when the logic has intentionally skipped writing the task configs to the backing topic.
This change will make fixes targeted at the task change logic also apply to the StandaloneHerder automatically, and prevent the divergence of the two code paths.
Additionally, this PR removes a ClusterConfigState method that was only in use by the StandaloneHerder.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #13287: MINOR: Refactor task change logic to AbstractHerder, reuse for standalone mode.
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13287:
URL: https://github.com/apache/kafka/pull/13287#discussion_r1120511592
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java:
##########
@@ -427,9 +427,8 @@ private void updateConnectorTasks(String connName) {
}
List<Map<String, String>> newTaskConfigs = recomputeTaskConfigs(connName);
- List<Map<String, String>> oldTaskConfigs = configState.allTaskConfigs(connName);
- if (!newTaskConfigs.equals(oldTaskConfigs)) {
Review Comment:
This logic seems significantly simpler than what's currently used in `DistributedHerder:: reconfigureConnector ` and moved to `AbstractHerder::taskConfigsChanged` in this PR.
Why not keep this logic instead of removing `ConfigClusterState::allTaskConfigs`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java:
##########
@@ -1981,6 +1966,8 @@ private void reconfigureConnector(final String connName, final Callback<Void> cb
}
});
}
+ } else {
+ log.debug("Skipping reconfiguration of connector {} as generated configs appear unchanged", connName);
Review Comment:
Why not move this into `AbstractHerder::taskConfigsChanged` as well, so that it's picked up in both standalone and distributed mode?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante merged pull request #13287: MINOR: Refactor task change logic to AbstractHerder, reuse for standalone mode.
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #13287:
URL: https://github.com/apache/kafka/pull/13287
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on a diff in pull request #13287: MINOR: Refactor task change logic to AbstractHerder, reuse for standalone mode.
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13287:
URL: https://github.com/apache/kafka/pull/13287#discussion_r1120610035
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java:
##########
@@ -427,9 +427,8 @@ private void updateConnectorTasks(String connName) {
}
List<Map<String, String>> newTaskConfigs = recomputeTaskConfigs(connName);
- List<Map<String, String>> oldTaskConfigs = configState.allTaskConfigs(connName);
- if (!newTaskConfigs.equals(oldTaskConfigs)) {
Review Comment:
This logic is simpler, but only because `ClusterConfigState::allTaskConfigs` and `List::equals` are abstracting some of the complexity.
There are multiple reasons I decided to keep the DistributedHerder implementation instead of the Standalone herder:
1. The DistributedHerder implementation allows us to provide more detailed debug logs about how the task configs are different, in a way that `List::equals` hides. Since distributed mode is more common, I figured adding those logs to standalone was better than taking them away from distributed mode.
2. The `taskConfig` and `allTaskConfigs` are duplicating one another to some degree (they're both retrieving task configs, they're both applying transformations) but we cannot remove `taskConfig` as it is used in lots of other areas of the herder.
3. The `allTaskConfigs` is currently performing a naive search over all known task configs, and while we could fix that, it requires a larger change with higher regression risk.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #13287: MINOR: Refactor task change logic to AbstractHerder, reuse for standalone mode.
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13287:
URL: https://github.com/apache/kafka/pull/13287#discussion_r1120636637
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java:
##########
@@ -427,9 +427,8 @@ private void updateConnectorTasks(String connName) {
}
List<Map<String, String>> newTaskConfigs = recomputeTaskConfigs(connName);
- List<Map<String, String>> oldTaskConfigs = configState.allTaskConfigs(connName);
- if (!newTaskConfigs.equals(oldTaskConfigs)) {
Review Comment:
I think the most convincing argument here is point 1; any duplication of work that isn't inevitable can be addressed to the same degree regardless of which class contains the logic, and I don't think the risk of improving the naive search is super high.
Anyways, sounds good to me 👍
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org