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

Fix an assert being triggered when no metrics matched on the client side during send push telemetry call #4826

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

pranavrth
Copy link
Member

No description provided.

@emasab
Copy link
Collaborator

emasab commented Aug 29, 2024

Closes #4833

tests/0150-telemetry_mock.c Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@pranavrth pranavrth merged commit 9416dd8 into master Aug 30, 2024
2 checks passed
@pranavrth pranavrth deleted the dev_fix-telemetry-assert branch August 30, 2024 08:33
# librdkafka v2.5.0

> [!WARNING]
This version has introduced a regression in which an assert is triggered during **PushTelemetry** call. This happens when no metric is matched on the client side among those requested by broker subscription.
Copy link
Member

Choose a reason for hiding this comment

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

This section could be more tightly worded. I don't think it's a regression. It's a critical defect which is only exposed when a long list of conditions are met.

I wonder if the list of conditions could be worded the other way around. Essentially, the problem only occurs if all of these are true.

Copy link
Member Author

Choose a reason for hiding this comment

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

This section could be more tightly worded. I don't think it's a regression. It's a critical defect which is only exposed when a long list of conditions are met.

I didn't understand what exactly needs to be done here. Are we talking about the regression part here?

I wonder if the list of conditions could be worded the other way around. Essentially, the problem only occurs if all of these are true.

Do you mean that we specify the scenario in which the problem occurs instead of specifying the scenario in which the problem doesn't occur?

Is something like this better:

This version has introduced a critical defect which crashes the client by hitting an assert during **PushTelemetry** call. This happens when no metric is matched on the client side among those requested by broker subscription. This happens in a specific scenario in which all the following conditions are met:
1) Broker supports KIP-714
2) KIP-714 feature is enabled on the broker side
3) KIP-714 feature is enabled on the client side. Enabled by default.
4) Broker has some subscription configured the client
5) None of the subscription matched on the client side.
If any of the above condition doesn't match for your case, you are good to use this version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Change these phrases:
4) Broker has a subscription assigned to the client
5) None of the subscription metrics matched on the client side.
any of the above conditions

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.

4 participants