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

client_async: Allow throwing an exception upon socket error during wakeup #134

Conversation

wbarnha
Copy link
Owner

@wbarnha wbarnha commented Mar 8, 2024

Motivation

When wakeup() is called, we sometime notice that we get an endless prints:
"Unable to send to wakeup socket!".
Those prints are spamming the logs.

Description

This commit aims to address it by allowing restating the application via an intentional exception raise.
This behavior is configurable and its default is backward compatible.

We see this issue with kafka-python 2.0.2


This change is Reviewable

shimonturjeman and others added 3 commits June 26, 2023 20:46
wakeup

When wakeup() is called, we sometime notice that we get
an endless prints:
"Unable to send to wakeup socket!".

Those prints are spamming the logs.
This commit aims to address it by allowing restating the
application via an intentional exception raise.
This behavior is configurable and its default is backward compatible.

Signed-off-by: shimon-armis <shimon.turjeman@armis.com>
@shimon-armis
Copy link

@wbarnha Nice!
So, what's missing for merging this (I didn't see any assigned reviewers, so not sure whose approval you're waiting for?).

@wbarnha wbarnha merged commit 54cbd63 into wbarnha:master Mar 10, 2024
21 checks passed
@wbarnha
Copy link
Owner Author

wbarnha commented Mar 10, 2024

@wbarnha Nice! So, what's missing for merging this (I didn't see any assigned reviewers, so not sure whose approval you're waiting for?).

Just wanted to reread it one nore time. LGTM!

@shimon-armis
Copy link

@wbarnha Is this going to be merged also to the dpkp/kafka-python repo?

@wbarnha
Copy link
Owner Author

wbarnha commented Mar 10, 2024

@wbarnha Is this going to be merged also to the dpkp/kafka-python repo?

Yes, however, the main kafka-python repository is going to contain a set of very carefully managed releases. I only want to start merging more things in once I have full control over the release schedule of the project.

The PRs that have already been merged are presumably uncontroversial to the parties to would object to me creating a release. I apologize for not being upfront, there are some things going on behind the scenes that bar me from going wild on merging things. I have a lax policy on accepting PRs because it encourages community interaction and the dopamine hit I presume people get from seeing their code get merged motivates them further. I do not like standing in the way of such gratification since it's one of the main reasons I originally got involved with open source contributions. I need all the help I can get in terms of maintaining projects.

@shimon-armis
Copy link

@wbarnha Is this going to be merged also to the dpkp/kafka-python repo?

Yes, however, the main kafka-python repository is going to contain a set of very carefully managed releases. I only want to start merging more things in once I have full control over the release schedule of the project.

The PRs that have already been merged are presumably uncontroversial to the parties to would object to me creating a release. I apologize for not being upfront, there are some things going on behind the scenes that bar me from going wild on merging things. I have a lax policy on accepting PRs because it encourages community interaction and the dopamine hit I presume people get from seeing their code get merged motivates them further. I do not like standing in the way of such gratification since it's one of the main reasons I originally got involved with open source contributions. I need all the help I can get in terms of maintaining projects.

Thanks for the honest answer.
I actually got involved with open source projects with the same reason.
I do understand the caution taken when merging code into master, though.
In any case, do you have any estimation when will it be merged to the dpkp/kafka-python?

@wbarnha
Copy link
Owner Author

wbarnha commented Mar 10, 2024

@wbarnha Is this going to be merged also to the dpkp/kafka-python repo?

Yes, however, the main kafka-python repository is going to contain a set of very carefully managed releases. I only want to start merging more things in once I have full control over the release schedule of the project.
The PRs that have already been merged are presumably uncontroversial to the parties to would object to me creating a release. I apologize for not being upfront, there are some things going on behind the scenes that bar me from going wild on merging things. I have a lax policy on accepting PRs because it encourages community interaction and the dopamine hit I presume people get from seeing their code get merged motivates them further. I do not like standing in the way of such gratification since it's one of the main reasons I originally got involved with open source contributions. I need all the help I can get in terms of maintaining projects.

Thanks for the honest answer. I actually got involved with open source projects with the same reason. I do understand the caution taken when merging code into master, though. In any case, do you have any estimation when will it be merged to the dpkp/kafka-python?

Either when my PEP 541 request gets accepted (I'll open one tomorrow, I don't have the energy to go through the internal email chain I've had with past maintainers), or if Dana gets back to me. We've tried reaching him multiple times but has been MIA since December.

@wbarnha
Copy link
Owner Author

wbarnha commented Mar 10, 2024

If the answer seems vague, that's because it is. I don't know either and I apologize for the lack of clarity.

@shimon-armis
Copy link

If the answer seems vague, that's because it is. I don't know either and I apologize for the lack of clarity.

That's o.k :)
I appreciate your honesty and your prompt response.

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