Skip to content

KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown#13700

Closed
machi1990 wants to merge 2 commits intoapache:trunkfrom
machi1990:KAFKA-14959
Closed

KAFKA-14959: remove delayed queue and exempt sensors during ClientQuota and ClientRequestQuota managers shutdown#13700
machi1990 wants to merge 2 commits intoapache:trunkfrom
machi1990:KAFKA-14959

Conversation

@machi1990
Copy link
Copy Markdown
Contributor

Follows up on #13623 (comment)

Committer Checklist (excluded from commit message)

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

@machi1990
Copy link
Copy Markdown
Contributor Author

@rajinisivaram @dajac please review when you've the chance

@machi1990
Copy link
Copy Markdown
Contributor Author

I am wondering if these sensors that are dynamically created should be removed as well: https://github.com/apache/kafka/blob/ca11a87e86e8e3c65043f747f35cae770b1efb7c/core/src/main/scala/kafka/server/ClientQuotaManager.scala#L393-L402 looking for suggestion here

@machi1990
Copy link
Copy Markdown
Contributor Author

@divijvaidya since you worked on #13623, would you be open to give this PR a review? Thanks

@divijvaidya divijvaidya self-assigned this Jun 15, 2023
Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. It's worth noting that the ClientQuotaManagers are shutdown on broker shutdown and hence, it's not a big deal if we are leaking metrics right now. Nevertheless, it's good practice to close metrics properly and remove them from the JMX server.

Comment thread core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another validation we can perform is to ensure that
assertTrue(metrics.metrics().isEmpty) at @AfterEach of BaseClientQuotaManagerTest

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked onto this and apparently the metrics.metrics() List is never empty because this metric https://github.com/apache/kafka/blob/ca11a87e86e8e3c65043f747f35cae770b1efb7c/clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java#L180 isn't at the moment removed when we Metrics#close() is called.

Is that to be expected? If not, I can create a separate JIRA and address it separate from this PR.

Comment thread core/src/main/scala/kafka/server/ClientQuotaManager.scala Outdated
@machi1990
Copy link
Copy Markdown
Contributor Author

Thank you for the PR. It's worth noting that the ClientQuotaManagers are shutdown on broker shutdown and hence, it's not a big deal if we are leaking metrics right now. Nevertheless, it's good practice to close metrics properly and remove them from the JMX server.

Thanks for the review @divijvaidya I've addressed all of your comments except for one where I've left a suggested course of action. Let me know what you think

Comment thread core/src/main/scala/kafka/server/ClientQuotaManager.scala Outdated
Comment thread core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala Outdated
Comment thread core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala Outdated
@divijvaidya
Copy link
Copy Markdown
Member

I wanted to let you know that this is on my radar and I need a few more days to get to this.

@machi1990
Copy link
Copy Markdown
Contributor Author

I wanted to let you know that this is on my radar and I need a few more days to get to this.

Thanks @divijvaidya

@machi1990
Copy link
Copy Markdown
Contributor Author

Hi @divijvaidya friendly ping on this PR, I know you are busy preparing 3.5.1 release, so whenever you've some free cycle, please review, thanks.

@machi1990
Copy link
Copy Markdown
Contributor Author

I've limited bandwidth thus I won't be able to carry this work forward. I'll happily close this MR and unassign myself from the JIRA for someone else to take it over.

@machi1990 machi1990 closed this Aug 24, 2023
@machi1990 machi1990 deleted the KAFKA-14959 branch August 24, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants