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.

Sign up for GitHub

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 segfault with describe_topics and flaky connection #1692

Merged
merged 5 commits into from
May 7, 2024
Merged

Fix segfault with describe_topics and flaky connection #1692

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
811167d
Fix segfault with describe_topics and flaky connection
lpsinger Dec 15, 2023
01d91f0
Revert "Fix segfault with describe_topics and flaky connection"
lpsinger Mar 20, 2024
8b5e60d
Fix segfault with describe_topics and flaky connection
lpsinger Mar 20, 2024
d1ab445
Update src/confluent_kafka/src/confluent_kafka.c
lpsinger Apr 24, 2024
6e48741
Undo all changes except for the one in c_Node_to_py
lpsinger May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/confluent_kafka/src/confluent_kafka.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,7 +1620,10 @@ PyObject *c_Node_to_py(const rd_kafka_Node_t *c_node) {

cfl_PyDict_SetInt(kwargs, "id", rd_kafka_Node_id(c_node));
cfl_PyDict_SetInt(kwargs, "port", rd_kafka_Node_port(c_node));
cfl_PyDict_SetString(kwargs, "host", rd_kafka_Node_host(c_node));
if (rd_kafka_Node_host(c_node))
Copy link
Contributor

@emasab emasab Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Now I have a doubt that the fix should be here in CKPy, because this uses the Metadata response too for the replicas and isrs and it uses the Metadata host here (not nullable)
https://github.com/apache/kafka/blob/e8cd661becb08279702f1670bb8f1d816537adea/clients/src/main/resources/common/message/MetadataResponse.json#L54
And broker metadata must be present for all the indexed brokers. Checking librdkafka code

Copy link
Contributor

@emasab emasab Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, here it can be None because it's possible not all the brokers are returned from the Metadata call. I'm not sure if this expected from the broker because the documentation says "Each broker in the response". And in that case LK just sets the broker id here
https://github.com/confluentinc/librdkafka/blob/c95d2ee950bda779c63843d376aab1d90ed8dfa9/src/rdkafka_aux.c#L281

In the list_topics API, instead, only the broker id list is returned in the replicas and isrs fields so the broker list in ClusterMetadata only contains complete BrokerMetadata objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems it's the expected behavior. I've created a PR for changing the documentation here.
An update: in Go and .NET it doesn't happen because it either uses C.GoString or PtrToStringUTF8 that returns nil or null.

cfl_PyDict_SetString(kwargs, "host", rd_kafka_Node_host(c_node));
else
PyDict_SetItemString(kwargs, "host", Py_None);
if((rack = rd_kafka_Node_rack(c_node)))
cfl_PyDict_SetString(kwargs, "rack", rack);

Expand Down