Add set_sasl_credentials binding across clients #1511
Conversation
This binding calls internally rd_kafka_sasl_set_credentials. This binding is needed across consumers, producers, and adminclient.
milindl
force-pushed
the
dev_set_sasl_credentials
branch
from
517c785 to
5f38678
Compare
milindl
force-pushed
the
dev_set_sasl_credentials
branch
from
5f38678 to
b0f0633
Compare
| return NULL; | ||
| } | ||
|
|
||
| error = rd_kafka_sasl_set_credentials(self->rk, username, password); |
There was a problem hiding this comment.
Check if we need to release GIL.
There was a problem hiding this comment.
We are actually doing some operations except just copying of credentials under the sasl.lock that librdkafka has.
I'm not sure how time consuming they are, so I released the GIL
There was a problem hiding this comment.
I think release the GIL is correct approach here.
@emasab - What is your thoughts?
There was a problem hiding this comment.
Yes, better to use CallState_begin and CallState_end because it acquires the mutex rk->rk_conf.sasl.lock and other locks in rd_kafka_all_brokers_wakeup that are blocking operations
There was a problem hiding this comment.
Great, merging this now since there's agreement to release the GIL
| return NULL; | ||
| } | ||
|
|
||
| error = rd_kafka_sasl_set_credentials(self->rk, username, password); |
There was a problem hiding this comment.
I think release the GIL is correct approach here.
@emasab - What is your thoughts?
This binding calls internally rd_kafka_sasl_set_credentials. This binding is needed across consumers, producers, and adminclient.
This binding calls internally rd_kafka_sasl_set_credentials.
This binding is needed across consumers, producers, and adminclient.
Since this binding is required by all three bindings, I've added it in a common file.
If I need to create a new file, or a new test file, please let me know.