-
Notifications
You must be signed in to change notification settings - Fork 139
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
add read buffer to improve performance #431
add read buffer to improve performance #431
Conversation
I just wanted to say: THANK YOU FOR HELPING DEVELOP THIS! This outright fixes performance issues when clients have a very long output, and is even noticeable in the general case. Maintainers, please merge this PR ASAP! Many thanks again to uruun, what a legend for helping with this fix |
Just found out an issue that this will fail with UnicodeDecodeError - unexpected end of data. Will fix soon. |
@rticau should I test it on local environment instead of CI? It seems it cannot find a Ubuntu 18 runner. I would post the results here. |
@uruun We are trying to figure out why tests are not running. But meanwhile I guess it wouldn't hurt if you could run the tests yourself. Thanks! |
Any update on this pull request? Seems like the CI jobs just need to pass and this can be merged, right? |
@rticau , other folks: Can we have this merged? I'm happy to help provide testing info or debug the CI builds. This is a really fantastic improvement. |
@scott-carrion Hi. This was not merged as the tests did not pass and I really do not have the time to check. If you can take a look, it would be great. |
@rticau , sorry, I haven't had time to either. Is there a way to re-run the CI tests so I can look at the logs and debug? |
@scott-carrion I think a small commit would re-trigger the tests (I didn't find an option to trigger manually). |
@rticau I pushed a small commit with updated param doc string |
Turns out there were some issues, I just pushed fixes, I hope now it passes :) |
Some issues I can still see:
|
Hi, sorry, I had to merge a big PR for dropping Python2 and Jython support - which also included major restructuring of the entire code. Could you maybe reintegrate your changes in to master? I hope it is just integrating the changes in to Thanks |
bc8a93d
to
155de41
Compare
I moved the changes to client.py . Lets try now |
Add a read buffer to improve performance of read_until and derived functions.
Should fix #425 - I used the tests included in the issue; the execution time of Read Until and pure paramiko is comparable. When used in internal tests I did, it reduced execution time from minutes to seconds.
Closes #425