confluent-kafka-python icon indicating copy to clipboard operation
confluent-kafka-python copied to clipboard

Fix segfault with describe_topics and flaky connection

Open lpsinger opened this issue 1 year ago • 8 comments

If you call describe_topics on a flaky connection, sometimes the admin client reply has the host set to a null pointer. When this occurs, instead of segfaulting, report the host as None.

lpsinger avatar Dec 15 '23 22:12 lpsinger

CLA assistant check
All committers have signed the CLA.

cla-assistant[bot] avatar Dec 15 '23 22:12 cla-assistant[bot]

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

cla-assistant[bot] avatar Dec 15 '23 22:12 cla-assistant[bot]

Thanks for the PR.

Even though the changes seems correct, I think we should not change the base implementation which is being used in alot of APIs. Some of the values of other APIs must not be NULL. I think its better to handle the scenario in this way - https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/src/Admin.c#L3198-L3201.

I am also checking if it should be possible to get the host information as NULL or should we handle it as an error instead. But in the meantime you can rebase the branch and do the change mentioned above if you have time. Otherwise, you can wait for my revert.

pranavrth avatar Mar 08 '24 08:03 pranavrth

But in the meantime you can rebase the branch and do the change mentioned above if you have time.

Done.

lpsinger avatar Mar 20 '24 17:03 lpsinger

/sem-approve

pranavrth avatar Apr 24 '24 09:04 pranavrth

@emasab, what else do you want me to do with this?

lpsinger avatar May 02 '24 16:05 lpsinger

@lpsinger after reviewing all the cases only in c_Node_to_py it's needed to check for NULL values because it takes the nodes from the Metadata request and some of then can be absent, so without host, when the broker is down and not listed there.

Could you leave only that change?

emasab avatar May 03 '24 07:05 emasab

Could you leave only that change?

Done. Would you like me to squash the changes?

lpsinger avatar May 03 '24 20:05 lpsinger

/sem-approve

pranavrth avatar May 06 '24 08:05 pranavrth

/sem-approve

pranavrth avatar May 07 '24 08:05 pranavrth

/sem-approve

pranavrth avatar May 07 '24 11:05 pranavrth

You're very welcome!

lpsinger avatar May 07 '24 12:05 lpsinger