Conversation
|
This is for I updated |
ewencp
left a comment
ewencp
left a comment
There was a problem hiding this comment.
one question, but iirc these are from some earlier branch anyway. as long as things match up ok, LGTM
| sftp_cmds = """ | ||
| put %s %s | ||
| """ % (local_path, remote_path) | ||
| cmd("Uploading artifacts in %s to your Apache home directory" % root, "sftp -b - %s@home.apache.org" % apache_id, stdin=sftp_cmds) |
There was a problem hiding this comment.
Is there a reason we needed to rearrange this? I think it was already slow and when I had it doing individual commands for each action it took even longer.
There was a problem hiding this comment.
Yeah it is very slow and put -r should be much faster. I recall there is comment that says we can not do put -r due to bug in some version. put -r in sftp works for me on my RHEL 7 desktop. I might make sense to provide a commented code path in the release.py so that we can optionally choose to use put -r if it is supported.
I recall Matthias said in the other PR that this change is to make the sftp upload more interactive.
There was a problem hiding this comment.
The upload takes long, and the script seems to be frozen at this point. Re-arranging allows to report progress and makes the script more "interactive" (it prints stuff regularly). It might make the upload slower, but it's IMHO a better tradeoff. We put the same changes in the other PRs that are already merged.
I actually killed the script multiple times preparing the bug-fix releases because I thought it's dead and started to investigate, until I found out it's just doing the upload in the background and it takes forever. Overall, I wasted more time by this, that the slow down of reporting progress during upload.
It's of course a personal preference and if you don't like it at all, we can go back to the old behavior. However, the script is not used on a daily bases and thus I think, the performance impact is not too important compared to improved user experience.
There was a problem hiding this comment.
makes sense, probably ideal solution would be to just get the output from commands printing incrementally instead of all at once at the end (or something more efficient than the sftp we are currently stuck with...).
There was a problem hiding this comment.
probably ideal solution would be to just get the output from commands printing incrementally instead of all at once at the end
Cannot follow here. With the change, we call sftp per file and report progress per file.
There was a problem hiding this comment.
it's fine as is, I was just saying that if we could stream a single sftp processes output, that would be faster & more efficient.
There was a problem hiding this comment.
#9070 apparently reduces the time from 2 hours to 5 minutes. sftp per file seems pretty slow even for something that is not used frequently. Not sure if it became slower over time or something specific to John's connection though. :)
There was a problem hiding this comment.
It was always slow. This PR only made the script print stuff to stdout regularly--before this PR, the script was uploading silently for a loooong time and one could think it froze (even if it was just doing/uploading stuff). This PR was not to make the upload faster, just to make the script appear alive :)
Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, Dong Lin <dolin@linkedin.com>
|
Merged to |
4 participants
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)