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

add read buffer to improve performance #431

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

uruun
Copy link

@uruun uruun commented Jun 30, 2023

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.

@scott-carrion
Copy link

scott-carrion commented Jul 7, 2023

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

@uruun uruun marked this pull request as draft July 12, 2023 13:21
@uruun
Copy link
Author

uruun commented Jul 12, 2023

Just found out an issue that this will fail with UnicodeDecodeError - unexpected end of data. Will fix soon.

@uruun uruun marked this pull request as ready for review July 14, 2023 08:59
@uruun
Copy link
Author

uruun commented Jul 17, 2023

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

@rticau
Copy link
Contributor

rticau commented Jul 17, 2023

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

@scott-carrion
Copy link

Any update on this pull request? Seems like the CI jobs just need to pass and this can be merged, right?

@scott-carrion
Copy link

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

@rticau
Copy link
Contributor

rticau commented Dec 7, 2023

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

@scott-carrion
Copy link

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

@rticau
Copy link
Contributor

rticau commented Apr 25, 2024

@scott-carrion I think a small commit would re-trigger the tests (I didn't find an option to trigger manually).

@uruun
Copy link
Author

uruun commented Apr 25, 2024

@rticau I pushed a small commit with updated param doc string

@uruun
Copy link
Author

uruun commented Apr 25, 2024

Turns out there were some issues, I just pushed fixes, I hope now it passes :)

@uruun
Copy link
Author

uruun commented Apr 25, 2024

Some issues I can still see:

  • The entire build job seems to be failing because robotstatuschecker 1.x uses deprecated functions from robotframework. It works after I installed robotstatuschecker >2.x but it dropped python 2 support. No idea if it will be an issue to use newer robotstatuschecker upstream.
  • Some tests are failing because of paramiko (probably this New to Python and Paramiko. Getting ValueError("q must be exactly 160, 224, or 256 bits long") paramiko/paramiko#2048)
  • Some tests are failing and it's expected but robotstatuschecker fails to parse them because it doesn't work at all
  • No idea about Jython, I didn't test it when I submitted this PR and I don't know why it fails.

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.

Read until very slow comparing to Paramiko
3 participants