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

SCP : limit the used bandwidth #208

Merged
merged 6 commits into from
Aug 18, 2015
Merged

Conversation

lguerin
Copy link
Contributor

@lguerin lguerin commented Aug 12, 2015

Hello,

I have a strong requirement to limit usage of bandwidth at application level when I upload files to a remote server.
According to the scp man page, I have added a '-l XXX' arg to the SCPEngine class.
Then, to keep the fluent aspect of your API, this new arg can (optionally) be set as below:

// Limit upload transfert at 128Ko/s
ssh.newSCPFileTransfer().bandwidthLimit(1024).upload(new FileSystemFile(src), "/tmp/");

A test case is provided into SCPFileTransferTest for upload and download features.
Thanks.

@hierynomus
Copy link
Owner

Hi! Thanks for the PR. Unfortunately, Travis claims that one test is failing. Can you have a look and fix it?

@lguerin
Copy link
Contributor Author

lguerin commented Aug 13, 2015

Hi,
I don't really know why the first build has failed, whereas the tests was ok on my local machine and with my travis instance: https://travis-ci.org/lguerin/sshj/builds/75394840
I just have change the logging level for tests in gradle file to get more informations about the exception cause if the error occurs again.

@@ -28,18 +28,23 @@
extends AbstractFileTransfer
implements FileTransfer {

/** Default bandwidth limit for SCP transfert in Kbit/s (-1 means unlimited) */
Copy link
Owner

Choose a reason for hiding this comment

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

typo in comment (transfert). Also is it kilobit, or kilobyte? The difference is a factor 8.

@lguerin
Copy link
Contributor Author

lguerin commented Aug 13, 2015

Sorry for the typo. It's kilobit/s, according to the scp man page.


private final char a;
private String v;
Copy link
Owner

Choose a reason for hiding this comment

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

Why the extra 'v' here? It is not used currently. The LIMIT seems to use it, but as it is an enum it is always the empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some scp options needs a key/value structure. The value is initialized with an empty string but can be set through the helper class SCPArguments. This the way I have used to set the value of the LIMIT arg.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh but this is even more evil, as this is an enum, setting the value for 1 process means that you're setting it (inadvertently) for all processes. Because there is only 1 instance of the same enum key in a JVM. So please refactor that and remove the setValue(..)

@hierynomus
Copy link
Owner

Thanks! Looks good now I think. Will do some testing on it, and then merge it in next week.

@lguerin
Copy link
Contributor Author

lguerin commented Aug 14, 2015

Nice, thank you.

hierynomus added a commit that referenced this pull request Aug 18, 2015
SCP : limit the used bandwidth
@hierynomus hierynomus merged commit 74a4012 into hierynomus:master Aug 18, 2015
@hierynomus
Copy link
Owner

Merged, thx!

@hierynomus hierynomus added this to the 0.14.0 milestone Aug 18, 2015
@lguerin lguerin deleted the bandwidth branch August 19, 2015 07:45
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.

2 participants