You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/18 19:06:06 UTC
[GitHub] [kafka] rhauch opened a new pull request #10153: KAFKA-12340: Fix potential resource leak in Kafka*BackingStore
rhauch opened a new pull request #10153:
URL: https://github.com/apache/kafka/pull/10153
These Kafka*BackingStore classes used in Connect have a recently-added deprecated constructor, which is **not** used within AK. However, this commit corrects a AdminClient resource leak if those deprecated constructors are used outside of AK. The fix simply ensures that the AdminClient created by the “default” supplier is always closed when the Kafka*BackingStore is stopped.
See #9780
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on a change in pull request #10153: KAFKA-12340: Fix potential resource leak in Kafka*BackingStore
Posted by GitBox <gi...@apache.org>.
rhauch commented on a change in pull request #10153:
URL: https://github.com/apache/kafka/pull/10153#discussion_r578702660
##########
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:
Good suggestion. Replaced this try-finally block with a simple `close()` call here an in the other 2 places.
----------------------------------------------------------------
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
[GitHub] [kafka] rhauch merged pull request #10153: KAFKA-12340: Fix potential resource leak in Kafka*BackingStore
Posted by GitBox <gi...@apache.org>.
rhauch merged pull request #10153:
URL: https://github.com/apache/kafka/pull/10153
----------------------------------------------------------------
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
[GitHub] [kafka] kkonstantine commented on a change in pull request #10153: KAFKA-12340: Fix potential resource leak in Kafka*BackingStore
Posted by 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