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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to