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

Unit tests push telemetry encode decode #4440

Merged
merged 4 commits into from
Sep 29, 2023
Merged

Conversation

anchitj
Copy link
Member

@anchitj anchitj commented Sep 20, 2023

No description provided.

@anchitj anchitj force-pushed the dev_kip_714_push_ut branch from 7ec933e to 3a91abe Compare September 25, 2023 07:37
@anchitj anchitj force-pushed the dev_kip_714_push_ut branch from 3a91abe to 1f93898 Compare September 25, 2023 07:38
@anchitj anchitj marked this pull request as ready for review September 25, 2023 08:51
@anchitj anchitj requested a review from milindl September 25, 2023 08:51
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

mostly looks good, a few comments.

also later on, we should remove the fprintfs and use RD_UT_SAY or something similar, but leaving that in as the function is used for debugging now

return status;
}

void clear_unit_test_data() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark these methods 'static' , everything that's used as a helper function for testing, or non-top level test functions, it's best to make static

unit_test_data.metric_time = 0;
}

bool unit_test_telemetry_gauge() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool unit_test_telemetry_gauge() {
bool unit_test_telemetry_gauge(void) {

minor

RD_UT_PASS();
}

bool unit_test_telemetry_sum() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool unit_test_telemetry_sum() {
bool unit_test_telemetry_sum(void) {

minor


int unittest_telemetry_decode(void) {
int fails = 0;
fails += unit_test_telemetry_gauge();
Copy link
Contributor

Choose a reason for hiding this comment

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

Both these tests can be turned into one function with a few params, they just differ at one or two places and the rest is the same

uint64_t metric_time;
};

struct metric_unit_test_data unit_test_data;
Copy link
Contributor

Choose a reason for hiding this comment

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

since we use this global variable only in this file, declare it static.

@anchitj anchitj force-pushed the dev_kip_714_push_ut branch from f5f7521 to 243a4c4 Compare September 28, 2023 11:11
@anchitj anchitj merged commit 1216b39 into dev_kip_714 Sep 29, 2023
1 check passed
@anchitj anchitj deleted the dev_kip_714_push_ut branch September 29, 2023 07:11
@dtheodor
Copy link

dtheodor commented Oct 5, 2023

When do you plan to have KIP 714 working? Is there a roadmap published somewhere?

@milindl
Copy link
Contributor

milindl commented Oct 5, 2023

@dtheodor , KIP-714 is not yet accepted by Apache Kafka: https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability

So, there's no roadmap yet for librdkafka. We need to wait for acceptance and for it to make it to Apache Kafka before we can put it into librdkafka.

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.

3 participants