You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "nizhikov (via GitHub)" <gi...@apache.org> on 2023/01/21 11:02:52 UTC
[GitHub] [kafka] nizhikov opened a new pull request, #13144: KAFKA-14463 Invoke of ConnectorClientConfigOverridePolicy#close
nizhikov opened a new pull request, #13144:
URL: https://github.com/apache/kafka/pull/13144
`ConnectorClientConfigOverridePolicy` implements `AutoCloseable` but close method not called.
### 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 merged pull request #13144: KAFKA-14463 Invoke of ConnectorClientConfigOverridePolicy#close
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #13144:
URL: https://github.com/apache/kafka/pull/13144
--
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 #13144: KAFKA-14463 Invoke of ConnectorClientConfigOverridePolicy#close
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13144:
URL: https://github.com/apache/kafka/pull/13144#discussion_r1084433485
##########
connect/runtime/src/test/java/org/apache/kafka/connect/connector/policy/BaseConnectorClientConfigOverridePolicyTest.java:
##########
@@ -27,7 +27,7 @@
public abstract class BaseConnectorClientConfigOverridePolicyTest {
- protected abstract ConnectorClientConfigOverridePolicy policyToTest();
Review Comment:
Good eye!
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java:
##########
@@ -227,9 +227,14 @@ public class DistributedHerderTest {
private SinkConnectorConfig conn1SinkConfig;
private SinkConnectorConfig conn1SinkConfigUpdated;
private short connectProtocolVersion;
+ private boolean connectorClientConfigOverridePolicyClosed = false;
private final ConnectorClientConfigOverridePolicy
- noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy();
-
+ noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy() {
+ @Override
+ public void close() {
+ connectorClientConfigOverridePolicyClosed = true;
+ }
+ };
Review Comment:
I like this testing strategy overall, but I think this logic can go into a dedicated class, similar to the [SampleSinkConnector class](https://github.com/apache/kafka/blob/00e5803cd3af89011254e734232308618403309d/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/SampleSinkConnector.java) that we use for unit tests that require a sink connector.
The `SampleConnectorClientConfigOverridePolicy` class (feel free to rename) could then expose a public `boolean isClosed()` method, which would replace the `connectorClientConfigOverridePolicyClosed` field here.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java:
##########
@@ -151,6 +151,11 @@ protected void stopServices() {
this.configBackingStore.stop();
this.worker.stop();
this.connectorExecutor.shutdown();
+ try {
+ this.connectorClientConfigOverridePolicy.close();
+ } catch (Exception e) {
+ log.warn("Exception while stop connectorClientConfigOverridePolicy:", e);
+ }
Review Comment:
This can be a one-liner with the `Utils` class:
```suggestion
Utils.closeQuietly(this.connectorClientConfigOverridePolicy, "connector client config override policy");
```
--
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] nizhikov commented on a diff in pull request #13144: KAFKA-14463 Invoke of ConnectorClientConfigOverridePolicy#close
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13144:
URL: https://github.com/apache/kafka/pull/13144#discussion_r1084491140
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java:
##########
@@ -227,9 +227,14 @@ public class DistributedHerderTest {
private SinkConnectorConfig conn1SinkConfig;
private SinkConnectorConfig conn1SinkConfigUpdated;
private short connectProtocolVersion;
+ private boolean connectorClientConfigOverridePolicyClosed = false;
private final ConnectorClientConfigOverridePolicy
- noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy();
-
+ noneConnectorClientConfigOverridePolicy = new NoneConnectorClientConfigOverridePolicy() {
+ @Override
+ public void close() {
+ connectorClientConfigOverridePolicyClosed = true;
+ }
+ };
Review Comment:
Fixed.
--
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] nizhikov commented on pull request #13144: KAFKA-14463 Invoke of ConnectorClientConfigOverridePolicy#close
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #13144:
URL: https://github.com/apache/kafka/pull/13144#issuecomment-1400908925
@C0urante Thanks for the review. I've applied your suggestions. Please, take a look one more time.
--
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