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

Fix getting a read timeout for logs/attach with a tty and slow output #1959

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Mar 15, 2018

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-

@akki
Copy link

akki commented Oct 10, 2019

Would any of the maintainers like to review this PR?

What would help in resolving #931?

Copy link
Contributor

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

Looks good!

@akki
Copy link

akki commented Nov 11, 2019

@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 master?

@akki
Copy link

akki commented Nov 11, 2019

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.

@shin- shin- self-assigned this Nov 13, 2019
@TomasTomecek
Copy link
Contributor

@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 master?

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.

@CodingJonas
Copy link

@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.

@HaloKo4
Copy link

HaloKo4 commented Dec 23, 2020

@shin- is there a chance you can review?

@akki
Copy link

akki commented Dec 24, 2020

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.

@michaelfresco
Copy link

This PR would be so cool for Airflow and Docker Swarm. I hope this PR can be approved. Small ping to @ulyssessouza, @aiordache, @thaJeztah

@HaloKo4
Copy link

HaloKo4 commented Sep 4, 2021

hey @aiordache @ulyssessouza

could you please get some attention to this PR? :)
3+ years for 2 lines for code is not reasonable.
I don't know why Docker ignores this PR - you are making you customers life really difficult.

@HaloKo4
Copy link

HaloKo4 commented Sep 14, 2021

@ulyssessouza @aiordache @thaJeztah
Kindly ask for a comment, any comment.
What else are we to do to get attention from the repo maintainers? :(

@thaJeztah
Copy link
Member

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)

@HaloKo4
Copy link

HaloKo4 commented Sep 14, 2021

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.

@thaJeztah
Copy link
Member

It's 2 lines of code.

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.

Copy link
Collaborator

@ulyssessouza ulyssessouza left a 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 <segev208@gmail.com>
@segevfiner
Copy link
Contributor Author

Rebased.

@aiordache aiordache merged commit b27faa6 into docker:master Sep 17, 2021
@aiordache aiordache added this to the 5.0.3 milestone Sep 17, 2021
@HaloKo4
Copy link

HaloKo4 commented Sep 30, 2021

@aiordache when should we expect 5.0.3 to be released?

@aiordache
Copy link
Collaborator

@HaloKo4 Sorry for the delay, we'll have 5.0.3 out by Oct. 8th

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.

getting timeout after streaming logs for some time
10 participants