Skip to content

MINOR: update release.py #5374

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.

Sign up for GitHub

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

Merged
mjsax merged 1 commit into from
Jul 17, 2018
Merged

MINOR: update release.py #5374

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions release.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def sftp_mkdir(dir):
try:
cmd_str = """
cd %s
mkdir %s
-mkdir %s
""" % (basedir, dirname)
cmd("Creating '%s' in '%s' in your Apache home directory if it does not exist (errors are ok if the directory already exists)" % (dirname, basedir), "sftp -b - %s@home.apache.org" % apache_id, stdin=cmd_str, allow_failure=True)
except subprocess.CalledProcessError:
Expand Down Expand Up @@ -479,16 +479,16 @@ def select_gpg_key():
for root, dirs, files in os.walk(artifacts_dir):
assert root.startswith(artifacts_dir)

for file in files:
local_path = os.path.join(root, file)
remote_path = os.path.join("public_html", kafka_output_dir, root[len(artifacts_dir)+1:], file)
sftp_cmds += "\nput %s %s" % (local_path, remote_path)

for dir in dirs:
sftp_mkdir(os.path.join("public_html", kafka_output_dir, root[len(artifacts_dir)+1:], dir))

if sftp_cmds:
cmd("Uploading artifacts in %s to your Apache home directory" % root, "sftp -b - %s@home.apache.org" % apache_id, stdin=sftp_cmds)
for file in files:
local_path = os.path.join(root, file)
remote_path = os.path.join("public_html", kafka_output_dir, root[len(artifacts_dir)+1:], file)
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)

@ewencp ewencp Jul 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@lindong28 lindong28 Jul 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mjsax mjsax Jul 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@ewencp ewencp Jul 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@mjsax mjsax Jul 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@ewencp ewencp Jul 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@ijuma ijuma Aug 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#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. :)

@mjsax mjsax Aug 4, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 :)


with open(os.path.expanduser("~/.gradle/gradle.properties")) as f:
contents = f.read()
Expand Down Expand Up @@ -581,7 +581,7 @@ def select_gpg_key():
http://home.apache.org/~%(apache_id)s/kafka-%(rc_tag)s/javadoc/

* Tag to be voted upon (off %(dev_branch)s branch) is the %(release_version)s tag:
https://git-wip-us.apache.org/repos/asf?p=kafka.git;a=tag;h=%(rc_githash)s
https://github.com/apache/kafka/releases/tag/%(rc_tag)s

* Documentation:
http://kafka.apache.org/%(docs_version)s/documentation.html
Expand All @@ -591,7 +591,7 @@ def select_gpg_key():

* Successful Jenkins builds for the %(dev_branch)s branch:
Unit/integration tests: https://builds.apache.org/job/kafka-%(dev_branch)s-jdk8/<BUILD NUMBER>/
System tests: https://jenkins.confluent.io/job/system-test-kafka-%(dev_branch)s/<BUILD_NUMBER>/
System tests: https://jenkins.confluent.io/job/system-test-kafka/job/%(dev_branch)s/<BUILD_NUMBER>/

/**************************************

Expand Down