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

[bboyleonp666]

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]

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]

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

[bboyleonp666]

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

[bboyleonp666]

@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]

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]

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

[chia7712]

@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]

@bboyleonp666 any updates?

[bboyleonp666]

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

[bboyleonp666]

@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]

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]

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

[chia7712]

@bboyleonp666 thanks for updated patch

[chia7712]

@bboyleonp666 could you please fix conflicts?

[bboyleonp666]

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

[chia7712]

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

[bboyleonp666]

Exactly. Let me update this one.

[bboyleonp666]

Apply this one in commit fd93c6b