-
Notifications
You must be signed in to change notification settings - Fork 481
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
5977 memory benchmark test #6187
Conversation
Can one of the admins verify this patch? |
@landreev, I added this to the project board but kept it in the dev column for now. Feel free to move it over when you're ready for code review. |
…; may be obvious - but just in case; #5977)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@landreev I'm approving this pull request (looks good!) but I did add a couple of comments for some small things you could change if you want.
curl -O https://raw.githubusercontent.com/IQSS/dataverse-sample-data/memory-monitoring-experiments/ec2config_memory_test.yaml | ||
|
||
# run the script: | ||
./ec2-create-instance.sh -a ec2-memory-benchmark -b ${dataverse_branch} -r https://github.com/IQSS/dataverse.git -g ec2config_memory_test.yaml 2>&1 | tee create-instance.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard coding the IQSS repo here, should we allow any fork to be specified? We could always add this later, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it's not going to work with any other branch. On account of the bombing external tools, for example. We'll want to add this later, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good.
|
||
# install gnuplot, quietly: | ||
|
||
yum -y install gnuplot >/dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "yum install" to work, the script has to be run as root, right? Should we do a check for this above? Check to make sure the script is being run as the root user, I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other things in the script that need root. It is run by the parent script as "sudo ec2-memory-benchmark-remote.sh".
In the current setup, this second (remote) script should never be run by the end user directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't notice the sudo. Thanks.
select pages, then taking snapshots of the allocated classes on the | ||
heap and the garbage collection stats; see the issue for more info). | ||
|
||
In the curret implementation we are testing the two most used pages - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: curret
…ate-instance.sh (expect the ssh lines to appear multiple times! #5977)
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist