-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix getting a read timeout for logs/attach with a tty and slow output #1959
Conversation
Would any of the maintainers like to review this PR? What would help in resolving #931? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@TomasTomecek Since you created #931 and you find this PR to be its solution for you to merge it? Or can you tell what else we can do for it to get into |
We are affected in Airflow because of this issue. Although there are ways to hack around it but we would love it if we could fix this properly. |
Find a maintainer of this repository and work with the person on a review for this PR. I'm not a maintainer, only a contributor. Since @shin- already assigned this PR to himself, hopefully you'd get a maintainer review from him soon. |
@shin-, I hope it is okay to ping you. This MR is open for over two years now, what is blocking it? I would be happy to help in any way to get this merged. |
@shin- is there a chance you can review? |
This PR is near its 3 years anniversary without any activity from any repo maintainers. So I hope it is okay to ping @ulyssessouza @aiordache to get their attention as they seem to be the most active maintainers of this repo right now. |
This PR would be so cool for Airflow and Docker Swarm. I hope this PR can be approved. Small ping to @ulyssessouza, @aiordache, @thaJeztah |
could you please get some attention to this PR? :) |
@ulyssessouza @aiordache @thaJeztah |
I'm not a maintainer for this repository (and not very proficient on Python code), but perhaps one of the others can have a look @ulyssessouza @aiordache Looks like this PR needs a rebase though (I see there's a merge conflict) |
It's 2 lines of code. Assuming this PR is accepted I have no problem to open a new PR with it or the maintainers can take over. This is really small modification. If not accepted kindly explain how maintainers suggest to solve the problem. |
I understand it's a small change, and I see a similar approach is taken in other parts of this file, but it still needs to be reviewed. A single line change does not make a change less risky. As said; I'm not a Python coder, so I defer review to others mentioned. It would be good if @segevfiner could rebase this PR and resolve the merge conflict, otherwise it can't be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just needs a rebase.
Fixes docker#931 Signed-off-by: Segev Finer <[email protected]>
2de2c14
to
63618b5
Compare
Rebased. |
@aiordache when should we expect 5.0.3 to be released? |
@HaloKo4 Sorry for the delay, we'll have 5.0.3 out by Oct. 8th |
Fixes #931
P.S. There is another possible instance of getting an unexpected timeout in
attach_socket
since you return the socket from requests/urllib3 to the user, which will have a timeout set.@shin-