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

CUMULUS-3890: Address sftp provider multi-part/fastGet concern #3809

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

Conversation

Nnaga1
Copy link
Contributor

@Nnaga1 Nnaga1 commented Sep 26, 2024

Summary: Summary of changes

Addresses CUMULUS-XX: Develop amazing new feature

Changes

  • Detailed list or prose of changes
  • ...

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Ad-hoc testing - Deploy changes and test manually
  • Integration tests

@@ -112,7 +112,9 @@ export class SftpClient {

log.info(`Downloading ${remoteUrl} to ${localPath}`);

await this.sftp.fastGet(remotePath, localPath);
await this.sftp.fastGet(remotePath, localPath, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ticket AC mentions fastGet use is justified/confirmed as acceptable, and an alternative implementation is made configurable, with fastGet disabled by default.

how would I even make an alternate implementation available? like an option for them to have concurrency if their server can handle it, is there some process.env or other var I can use, a little confused about that

@@ -112,7 +112,9 @@ export class SftpClient {

log.info(`Downloading ${remoteUrl} to ${localPath}`);

await this.sftp.fastGet(remotePath, localPath);
await this.sftp.fastGet(remotePath, localPath, {
Copy link
Contributor Author

@Nnaga1 Nnaga1 Sep 26, 2024

Choose a reason for hiding this comment

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

fastGet use is justified/confirmed as acceptable: the other SFTP alternative would be the simple get, which isnt as performant and requires a bit more configuration and parameters to work properly. I think fastGet can continued to be used with some way or check on what the concurrency should be. If DAACs can handle concurrency than that can be left alone or configured, and if they can't it can be set to 0 by default. I was reading something about read/write streams as an alternative so that may be an option, but I think this is one as well.

@paulpilone paulpilone requested review from paulpilone and removed request for paulpilone October 1, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant