kamalcph commented on code in PR #17492:
URL: https://github.com/apache/kafka/pull/17492#discussion_r1800542225
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java:
##########
@@ -391,6 +391,8 @@ private void initializeResources() {
boolean topicCreated = false;
long startTimeMs = time.milliseconds();
Admin adminClient = null;
+
+ lock.writeLock().lock();
Review Comment:
IIRC, the reason for not taking `writeLock` before the creating of the Admin
client is that the topic exists/creation operation is costly when the
controller queue is busy. If we take a lock, then the callers of the RLMM might
be waiting for this operation to complete.
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java:
##########
@@ -567,9 +567,9 @@ public TopicBasedRemoteLogMetadataManagerConfig config() {
public void close() throws IOException {
// Close all the resources.
log.info("Closing topic-based RLMM resources");
- if (closing.compareAndSet(false, true)) {
- lock.writeLock().lock();
- try {
+ lock.writeLock().lock();
Review Comment:
Why do we need the `writeLock` in the close method? Can we avoid taking the
lock in the close method?
We already have AtomicBoolean `closing` to inform the callers that this
instance is about to be closed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
