Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#3017] Remove producer metrics when tenant is deleted #3038

Merged
merged 1 commit into from Jan 20, 2022
Merged

[#3017] Remove producer metrics when tenant is deleted #3038

merged 1 commit into from Jan 20, 2022

Conversation

calohmn
Copy link
Contributor

@calohmn calohmn commented Jan 19, 2022

This fixes #3017:
Kafka producer metrics regarding the telemetry, event and command-response topics of a tenant are removed when that
tenant got deleted.

@calohmn calohmn added the Client label Jan 19, 2022
@calohmn calohmn added this to the 1.11.0 milestone Jan 19, 2022
@calohmn calohmn added this to In progress in 1.11.0 via automation Jan 19, 2022
sophokles73
sophokles73 reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

wouldn't it make sense to add test cases that actually verify that the metrics are being removed?

@calohmn
Copy link
Contributor Author

calohmn commented Jan 19, 2022

Yes, I can add an integration test here.

@sophokles73
Copy link
Contributor

integration test? I was talking about the unit tests ...

@calohmn
Copy link
Contributor Author

calohmn commented Jan 19, 2022

Do you mean unit tests for testing whether KafkaProducerHelper#removeTopicMetrics gets invoked by means of the event bus notifications?
Or testing the actual metrics removal in that method?
I was thinking about the latter. For testing that, a proper org.apache.kafka.clients.producer.KafkaProducer instance is needed and its metrics field would need to get populated first by means of sending messages. I think that would need an integration test.

@sophokles73
Copy link
Contributor

I was referring to the former. Or isn't that helpful?

@calohmn
Copy link
Contributor Author

calohmn commented Jan 20, 2022

unit tests for testing whether KafkaProducerHelper#removeTopicMetrics gets invoked by means of the event bus notifications

Testing the actual invocation here would be quite cumbersome. I've added a basic test regarding consumer registration though.
I've also added an integration test for the KafkaProducerHelper#removeTopicMetrics method.

sophokles73
sophokles73 reviewed Jan 20, 2022
View reviewed changes
tests/src/test/java/org/eclipse/hono/tests/client/KafkaProducerHelperIT.java Outdated
*/
@Test
@Timeout(value = 10, timeUnit = TimeUnit.SECONDS)
public void testProducerTopicMetricsGetRemoved() throws InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it make more sense to test this in the context of actually deleting a tenant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a test with a different scope then. But I'm fine with adding a combined integration test like that. I've created KafkaBasedEventSenderIT now (and removed KafkaProducerHelperIT).

@sophokles73
Copy link
Contributor

@calohmn Did you change anything with your latest push?

@calohmn
Copy link
Contributor Author

calohmn commented Jan 20, 2022

@sophokles73 Only a minor improvement in KafkaProducerHelperIT. Still looking into writing an overall test...

@calohmn
[eclipse-hono#3017] Remove producer metrics when tenant is deleted.
5c97420 
Kafka producer metrics regarding the telemetry, event and
command-response topics of a tenant are removed when that
tenant got deleted.

Signed-off-by: Carsten Lohmann <carsten.lohmann@bosch.io>
sophokles73
sophokles73 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

I am not sure if the current integration test has more value than a simple unit test that verifies the removal of the metrics in the Kafka client. Thus, I (still) wonder why we need an IT at all. But if you want to keep it, by my guest ...

@calohmn
Copy link
Contributor Author

calohmn commented Jan 20, 2022

I am not sure if the current integration test has more value than a simple unit test that verifies the removal of the metrics in the Kafka client. Thus, I (still) wonder why we need an IT at all.

Testing of the actual removal of metrics can only be done with a proper org.apache.kafka.clients.producer.KafkaProducer instance (not a MockProducer) that actually connects to a Kafka broker and publishes records.
The IT makes sure that really all topic-related metrics are removed. If a newer Kafka client version introduces additional topic-metrics, the IT here will fail.

@calohmn calohmn merged commit 312eb2e into eclipse-hono:master Jan 20, 2022
1.11.0 automation moved this from In progress to Done Jan 20, 2022
@calohmn calohmn deleted the PR/remove_producer_metrics branch January 20, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophokles73 sophokles73 sophokles73 approved these changes

@ctron ctron Awaiting requested review from ctron

@dejanb dejanb Awaiting requested review from dejanb dejanb is a code owner

@kaniyan kaniyan Awaiting requested review from kaniyan kaniyan is a code owner

Assignees
No one assigned
Labels
Client
Projects
No open projects
1.11.0
Done
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

Obsolete Kafka Producer metrics should get removed to prevent memory leak
2 participants
@calohmn @sophokles73