[GitHub] [kafka] kkonstantine commented on a change in pull request #10153: KAFKA-12340: Fix potential resource leak in Kafka*BackingStore

[GitBox <gi...@apache.org>]

kkonstantine commented on a change in pull request #10153:
URL: https://github.com/apache/kafka/pull/10153#discussion_r578694124



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java
##########
@@ -140,7 +149,17 @@ public void start() {
     @Override
     public void stop() {
         log.info("Stopping KafkaOffsetBackingStore");
-        offsetLog.stop();
+        try {
+            offsetLog.stop();
+        } finally {
+            if (ownTopicAdmin != null) {
+                try {
+                    ownTopicAdmin.close();
+                } finally {
+                    ownTopicAdmin = null;

Review comment:
       same comment as above

##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaStatusBackingStore.java
##########
@@ -221,7 +230,17 @@ public void start() {
 
     @Override
     public void stop() {
-        kafkaLog.stop();
+        try {
+            kafkaLog.stop();
+        } finally {
+            if (ownTopicAdmin != null) {
+                try {
+                    ownTopicAdmin.close();
+                } finally {
+                    ownTopicAdmin = null;

Review comment:
       same as above

##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -291,7 +293,17 @@ public void start() {
     @Override
     public void stop() {
         log.info("Closing KafkaConfigBackingStore");
-        configLog.stop();
+        try {
+            configLog.stop();
+        } finally {
+            if (ownTopicAdmin != null) {
+                try {
+                    ownTopicAdmin.close();
+                } finally {
+                    ownTopicAdmin = null;
+                }

Review comment:
       I'd suggest avoiding this pattern. Requiring to set variables to `null` can be error prone. 
   
   Thankfully the `SharedTopicAdmin` class has an atomic variable that can sufficiently synchronize consecutive calls to `close` allowing only the first one to have the effect and ignoring any subsequent ones on the same object.




----------------------------------------------------------------
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.

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