KAFKA-17285: Consider using `Utils.closeQuietly` to replace `CoreUtils.swallow` when handling Closeable objects #16843

bboyleonp666 · 2024-08-09T00:37:57Z

Use Utils.closeQuietly to replace CoreUtils.swallow in kafka.network.Acceptor#closeAll.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

bboyleonp666 · 2024-08-09T00:53:04Z

Using Utils.closeQuietly to replace CoreUtils.swallow will also change the logging level from ERROR to WARNING, which makes sense to me, since the close() called before the thread starting is not supposed to happen. The warning message is enough for this behavior.

chia7712 · 2024-08-11T23:07:08Z

@bboyleonp666 could you please grep code base to fix all similar issues?

bboyleonp666 · 2024-08-13T10:22:47Z

Sure. Let me take a look at the code base.

bboyleonp666 · 2024-08-15T07:46:23Z

@chia7712 While handling this, I have noticed that there are lots of methods that uses shutdown() instead of close() when calling CoreUtils.swallow(). Currently, only close() is handled by Utils.closeQuietly() and I cannot find a method that can gracefully deal with shutdown() method. Are there any methods that can do this? If not, I wonder if we are going to add one similarly. Maybe name it shutdownQuietly?

chia7712 · 2024-08-18T18:05:05Z

Are there any methods that can do this? If not, I wonder if we are going to add one similarly. Maybe name it shutdownQuietly?

the shutdown is not a kind of interface, so I guess it would be hard to add "one helper" to handle "all" cases. Maybe this PR can focus on deal with the AutoClosable object.

bboyleonp666 · 2024-08-21T09:03:56Z

I agree with you. I have already merged the commits in trunk.

chia7712 · 2024-08-22T07:38:19Z

@bboyleonp666 please rebase code to fix build error. Also, please fix following usages:

kafka/core/src/test/scala/unit/kafka/zk/EmbeddedZookeeper.scala

Line 72 in 28cd713

CoreUtils.swallow(zookeeper.getZKDatabase.close(), this)

kafka/core/src/main/scala/kafka/admin/AclCommand.scala

Line 221 in 28cd713

finally CoreUtils.swallow(authZ.close(), this)

kafka/core/src/main/scala/kafka/network/SocketServer.scala

Line 420 in 28cd713

CoreUtils.swallow(channel.close(), logging, Level.ERROR)

kafka/core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

Line 684 in 28cd713

CoreUtils.swallow(reader.close(), logging)

kafka/core/src/main/scala/kafka/server/ControllerServer.scala

Line 70 in 28cd713

CoreUtils.swallow(zkClient.close(), logging)

kafka/core/src/main/scala/kafka/server/ZkAdminManager.scala

Line 550 in 28cd713

CoreUtils.swallow(createTopicPolicy.foreach(_.close()), this)

kafka/core/src/main/scala/kafka/server/ZkAdminManager.scala

Line 551 in 28cd713

CoreUtils.swallow(alterConfigPolicy.foreach(_.close()), this)

chia7712 · 2024-09-01T15:47:57Z

@bboyleonp666 any updates?

bboyleonp666 · 2024-09-03T10:03:18Z

I was on a long trip just back to work recently. I will update this soon, thanks for reminding.

chia7712

@bboyleonp666 thanks for your updates

core/src/test/scala/unit/kafka/zk/EmbeddedZookeeper.scala

bboyleonp666 · 2024-09-07T06:45:13Z

@bboyleonp666 please rebase code to fix build error. Also, please fix following usages:

kafka/core/src/test/scala/unit/kafka/zk/EmbeddedZookeeper.scala

Line 72 in 28cd713

CoreUtils.swallow(zookeeper.getZKDatabase.close(), this)

Since zookeeper.getZKDatabase is not a Closeable, we are not able to use Utils.closeQuietly() to replace it. I will keep it as it is.

kafka/core/src/main/scala/kafka/server/ZkAdminManager.scala

Line 550 in 28cd713

CoreUtils.swallow(createTopicPolicy.foreach(_.close()), this)

kafka/core/src/main/scala/kafka/server/ZkAdminManager.scala

Line 551 in 28cd713

CoreUtils.swallow(alterConfigPolicy.foreach(_.close()), this)

The lines here use foreach to close the resources, while CoreUtils.swallow() is not limited to an object type like Utils.closeQuietly() does. I wonder whether we should keep using foreach to keep the code concise and consistent or should we make it a for loop to achieve the same effect? What's your opinion? @chia7712

chia7712 · 2024-09-07T18:52:29Z

The lines here use foreach to close the resources, while CoreUtils.swallow() is not limited to an object type like Utils.closeQuietly() does. I wonder whether we should keep using foreach to keep the code concise and consistent or should we make it a for loop to achieve the same effect? What's your opinion? @chia7712

how about: createTopicPolicy.foreach(Utils.closeQuietly(_, "close create topic policy"))

bboyleonp666 · 2024-09-08T06:50:25Z

Looks great. Let me apply this change to all the instances that I can find.

…oseQuietly

core/src/main/scala/kafka/server/ControllerServer.scala

core/src/main/scala/kafka/server/SharedServer.scala

core/src/main/scala/kafka/server/ControllerServer.scala

chia7712

@bboyleonp666 thanks for updated patch

core/src/main/scala/kafka/server/BrokerServer.scala

core/src/main/scala/kafka/server/ControllerServer.scala

core/src/main/scala/kafka/server/KafkaServer.scala

core/src/main/scala/kafka/server/ZkAdminManager.scala

chia7712 · 2024-09-26T13:53:53Z

@bboyleonp666 could you please fix conflicts?

bboyleonp666 · 2024-09-28T08:42:25Z

Hi @chia7712, thank you. Just resolved and waiting for the checks.

chia7712 · 2024-09-29T17:54:05Z

core/src/main/scala/kafka/server/BrokerServer.scala

@@ -766,14 +766,13 @@ class BrokerServer(
        CoreUtils.swallow(socketServer.shutdown(), this)
      if (brokerTopicStats != null)
        CoreUtils.swallow(brokerTopicStats.close(), this)


BrokerTopicStats extends Closeable now, so you can apply Utils.closeQuietly, right?

Exactly. Let me update this one.

Apply this one in commit fd93c6b

Use Utils.closeQuietly to replace CoreUtils.swallow

77a4d68

bboyleonp666 marked this pull request as ready for review August 10, 2024 05:32

Use Utils.closeQuietly to replace CoreUtils.swallow: cont

f0dabb5

Merge branch 'trunk' into KAFKA-17285

967b601

Merge branch 'trunk' into KAFKA-17285

e13c873

bboyleonp666 force-pushed the KAFKA-17285 branch from cbd1e54 to e13c873 Compare September 4, 2024 15:36

chia7712 reviewed Sep 4, 2024

View reviewed changes

core/src/test/scala/unit/kafka/zk/EmbeddedZookeeper.scala Outdated Show resolved Hide resolved

bboyleonp666 added 2 commits September 7, 2024 14:06

Merge branch 'trunk' into KAFKA-17285

cb3bc46

replace CoreUtils.swallow with Utils.closeQuietly and fix bugs

9dc5d7d

replace foreach instances that applys CoreUtils.swallow with Utils.cl…

6a7d26e

…oseQuietly

chia7712 reviewed Sep 8, 2024

View reviewed changes

core/src/main/scala/kafka/server/ControllerServer.scala Outdated Show resolved Hide resolved

bboyleonp666 added 2 commits September 10, 2024 11:38

Merge branch 'trunk' into KAFKA-17285

893b555

make AutoCloseable use Utils.closeQuietly instead

ca091c9

bboyleonp666 commented Sep 10, 2024

View reviewed changes

core/src/main/scala/kafka/server/SharedServer.scala Show resolved Hide resolved

core/src/main/scala/kafka/server/SharedServer.scala Show resolved Hide resolved

core/src/main/scala/kafka/server/ControllerServer.scala Show resolved Hide resolved

chia7712 reviewed Sep 13, 2024

View reviewed changes

bboyleonp666 added 2 commits September 16, 2024 18:24

make brokerTopicStats implement AutoCloseable

21f9b18

Merge branch 'trunk' into KAFKA-17285

48dfffe

Merge branch 'trunk' into KAFKA-17285

a2dbd80

github-actions bot added core Kafka Broker storage Pull requests that target the storage module labels Sep 28, 2024

chia7712 added the ci-approved label Sep 28, 2024

chia7712 reviewed Sep 29, 2024

View reviewed changes

bboyleonp666 added 2 commits September 30, 2024 10:59

Merge branch 'trunk' into KAFKA-17285

b89a0e3

apply Utils.closeQuietly to brokerTopicStats

fd93c6b

chia7712 approved these changes Oct 3, 2024

View reviewed changes

chia7712 merged commit 8be808e into apache:trunk Oct 3, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17285: Consider using `Utils.closeQuietly` to replace `CoreUtils.swallow` when handling Closeable objects #16843

KAFKA-17285: Consider using `Utils.closeQuietly` to replace `CoreUtils.swallow` when handling Closeable objects #16843

bboyleonp666 commented Aug 9, 2024

bboyleonp666 commented Aug 9, 2024

chia7712 commented Aug 11, 2024

bboyleonp666 commented Aug 13, 2024

bboyleonp666 commented Aug 15, 2024

chia7712 commented Aug 18, 2024

bboyleonp666 commented Aug 21, 2024

chia7712 commented Aug 22, 2024

chia7712 commented Sep 1, 2024

bboyleonp666 commented Sep 3, 2024

chia7712 left a comment

bboyleonp666 commented Sep 7, 2024 •

edited

Loading

chia7712 commented Sep 7, 2024 •

edited

Loading

bboyleonp666 commented Sep 8, 2024

chia7712 left a comment

chia7712 commented Sep 26, 2024

bboyleonp666 commented Sep 28, 2024

chia7712 Sep 29, 2024

bboyleonp666 Sep 30, 2024

bboyleonp666 Sep 30, 2024 •

edited

Loading

KAFKA-17285: Consider using Utils.closeQuietly to replace CoreUtils.swallow when handling Closeable objects #16843

KAFKA-17285: Consider using Utils.closeQuietly to replace CoreUtils.swallow when handling Closeable objects #16843

Conversation

bboyleonp666 commented Aug 9, 2024

Committer Checklist (excluded from commit message)

bboyleonp666 commented Aug 9, 2024

chia7712 commented Aug 11, 2024

bboyleonp666 commented Aug 13, 2024

bboyleonp666 commented Aug 15, 2024

chia7712 commented Aug 18, 2024

bboyleonp666 commented Aug 21, 2024

chia7712 commented Aug 22, 2024

chia7712 commented Sep 1, 2024

bboyleonp666 commented Sep 3, 2024

chia7712 left a comment

Choose a reason for hiding this comment

bboyleonp666 commented Sep 7, 2024 • edited Loading

chia7712 commented Sep 7, 2024 • edited Loading

bboyleonp666 commented Sep 8, 2024

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 commented Sep 26, 2024

bboyleonp666 commented Sep 28, 2024

chia7712 Sep 29, 2024

Choose a reason for hiding this comment

bboyleonp666 Sep 30, 2024

Choose a reason for hiding this comment

bboyleonp666 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

KAFKA-17285: Consider using `Utils.closeQuietly` to replace `CoreUtils.swallow` when handling Closeable objects #16843

KAFKA-17285: Consider using `Utils.closeQuietly` to replace `CoreUtils.swallow` when handling Closeable objects #16843

bboyleonp666 commented Sep 7, 2024 •

edited

Loading

chia7712 commented Sep 7, 2024 •

edited

Loading

bboyleonp666 Sep 30, 2024 •

edited

Loading