Skip to content

KIP 714 with compression support#4721

Merged
Anchit Jain (anchitj) merged 99 commits intomasterfrom
dev_kip_714_mock_broker_integration_tests_master_merge_with_compression
Jul 8, 2024
Merged

KIP 714 with compression support#4721
Anchit Jain (anchitj) merged 99 commits intomasterfrom
dev_kip_714_mock_broker_integration_tests_master_merge_with_compression

Conversation

@anchitj
Copy link
Copy Markdown
Contributor

No description provided.

Milind L (milindl) and others added 30 commits July 25, 2023 12:06
* WIP:Push telemetry is being scheduled now.

* Push

* Compliation Fix

* Working

* Use UUID in PUSH

* Remove fprintf

* Changes

* Fix CONFIGURATION.md

* Fix size

* Update s2i bounds

---------

Co-authored-by: Milind L <[email protected]>
* Add broker selection and client termination

* Address review comments
* Serialise metrics using nanopb

* Move nanopb and opentelemetry inside src

* Add metrics.options file

* Remove unused includes

* Style fix

* Fix formatting

* Skip copyright check

* Add nanopb and opentelemetry in windows vcxproj

* Include headers directories in CMAKE

* Use flexver with PushTelemetry

* Fix memory leaks

* Change import path

* Use rd_bool_t everywhere

* Fix librdkafka.vcxproj

* Use rd_bool_t

* PR Feedback

* Add nanopb license

* Include opentelemtry license
* Support for delta temporality

* Style fix

* Fix bugs

* Fix memory leaks and formatting

* Fixes

* PR Feedback
* Add telemetry encode and decode unit tests

* Style fix

* Improve test

* PR Feedback
* Add max telemetry bytes

* Clear telemetry_max_bytes

* PR comments
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comments about encode and decode

Comment thread src/rdkafka_op.h Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry_encode.c
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry_decode.h Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of remaining comments

Comment thread src/rdkafka_broker.c
Comment thread src/rdkafka_int.h Outdated
Comment thread src/rdkafka_mock_handlers.c Outdated
Comment thread src/rdkafka_mock_handlers.c
Comment thread src/rdkafka_mock_handlers.c Outdated
Comment thread src/rdkafka_telemetry.c
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comments about unit tests and naming

Comment thread src/rdkafka_telemetry_decode.c
Comment thread src/rdkafka_telemetry_decode.c
Comment thread src/rdkafka_telemetry_decode.c
Comment thread src/rdkafka_telemetry_decode.c
Comment thread src/rdkafka_telemetry_decode.c Outdated
Comment thread src/rdkafka_telemetry_decode.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_mock_handlers.c
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copyright and includes comments

Comment thread src/rd.h
Comment thread src/rdkafka_telemetry_decode.c
Comment thread src/rdkafka_telemetry_decode.h Outdated
#ifndef _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H
#define _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H

#include "rdtypes.h"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add #include "rdkafka_int.h" as it's needed for rd_kafka_t

Suggested change
#include "rdtypes.h"
#include "rdkafka_int.h"
#include "rdtypes.h"

Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.h


#ifndef _RD_KAFKA_TELEMETRY_H_
#define _RD_KAFKA_TELEMETRY_H_
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Required for rd_kafka_t

Suggested change
#define _RD_KAFKA_TELEMETRY_H_
#define _RD_KAFKA_TELEMETRY_H_
#include "rdkafka_int.h"

Comment thread src/rdkafka_telemetry.h Outdated
Comment thread src/rdkafka_telemetry_decode.c Outdated
@emasab
Copy link
Copy Markdown
Contributor

Please add these to the list of Supported protocol versions in INTRODUCTION.md

| 71      | GetTelemetrySubscriptions     | 0          | 0              |
| 72      | PushTelemetry                 | 0          | 0              |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments about placeholders types in formatted strings

Comment thread src/rdkafka_telemetry_decode.c Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry_encode.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
Comment thread src/rdkafka_telemetry.c Outdated
@anchitj Anchit Jain (anchitj) force-pushed the dev_kip_714_mock_broker_integration_tests_master_merge_with_compression branch from 9616eba to 4732ff6 Compare July 8, 2024 06:44
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome work, thanks Anchit Jain (@anchitj) and Milind L (@milindl) !

@anchitj Anchit Jain (anchitj) merged commit 6eaf89f into master Jul 8, 2024
@anchitj Anchit Jain (anchitj) deleted the dev_kip_714_mock_broker_integration_tests_master_merge_with_compression branch July 8, 2024 07:47
@georgthegreat
Copy link
Copy Markdown

Hmm...
This PR introduced a mandatory dependency on nanopb runtime and opentelemetry protos generated with nanopb.
While there is nothing wrong with either of dependencies, having a switch to disable them would be nice.

Anchit Jain (@anchitj) Emanuele Sabellico (@emasab) what do you think?
Other depenndencies are configurable.

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.

5 participants