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

Made the name of environment variables consistent through benchmarking doc #740

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sauraank
Copy link
Contributor

@sauraank sauraank commented Feb 7, 2024

Description of change

In our Benchmarking doc, the names for environment variables S3_BUCKET_NAME, S3_BUCKET_TEST_PREFIX, S3_BUCKET_BENCH_FILE and S3_BUCKET_SMALL_BENCH_FILE across steps. This might create confusion for new user trying to run benchmark.

Made the names consistent across the steps for environment variables in used in benchmarking.

Relevant issues:
NA

Does this change impact existing behavior?

No. Just documentation edits.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank requested a review from arsh February 7, 2024 19:19
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 7, 2024 19:19 — with GitHub Actions Inactive
@@ -32,12 +32,12 @@ You can use the following steps.
--fuse-version 2 \
--with-fio --with-libunwind

2. Set environment variables related to the benchmark. There are four required environment variables you need to set in order to run the benchmark.
2. Set environment variables related to the benchmark. There are four required environment variables you need to set in order to run the benchmark. These variables can be set to any name, but be consistent with it in subsequent steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change these steps to first create the files needed and then populate the env variables?

Also, on line 45, there is a DOC-EXAMPLE-BUCKET which is undefined. Please add that to the set of variables to set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That seems more reasonable. Making the change. Thanks

@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:29 — with GitHub Actions Inactive
@sauraank sauraank requested a review from arsh February 8, 2024 14:30
@sauraank sauraank force-pushed the update_benchmark_documentation branch from 486b350 to 6336ab6 Compare February 8, 2024 14:35
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:35 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:36 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:36 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:36 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:36 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 8, 2024 14:36 — with GitHub Actions Inactive
@sauraank sauraank force-pushed the update_benchmark_documentation branch from 6336ab6 to 2f38025 Compare February 9, 2024 16:55
@sauraank sauraank temporarily deployed to PR integration tests February 9, 2024 16:55 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests February 9, 2024 16:55 — with GitHub Actions Inactive
Comment on lines +50 to +53
export S3_BUCKET_NAME=DOC-EXAMPLE-BUCKET
export S3_BUCKET_TEST_PREFIX=benchmark/
export S3_BUCKET_BENCH_FILE=bench100GB.bin
export S3_BUCKET_SMALL_BENCH_FILE=bench5MB.bin
Copy link
Contributor

Choose a reason for hiding this comment

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

I think step 2 should be creating those variables (what is now in step 3). Then, step 3 can refer to those variables (what is now in step 2).

Copy link
Contributor Author

@sauraank sauraank Feb 16, 2024

Choose a reason for hiding this comment

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

These environment variables are to run the benchmark script that we run in step 4. We earlier thought that these environment variables should be set once we have created the resource: #740 (comment)

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.

None yet

2 participants