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

Fix the threadPoolSize not working. #18

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Fix the threadPoolSize not working. #18

merged 3 commits into from
Aug 22, 2024

Conversation

iteyelmp
Copy link
Collaborator

@iteyelmp iteyelmp commented Aug 21, 2024

ethstorage/ethstorage-sdk#17 (comment)
#15 (review)

Fix the bug where the threadPoolSize parameter has no effect. The issue is caused by the parameter being passed as a string, which fails to be recognized correctly. As a result, the concurrency level defaults to the maximum, causing -6 and -12 to produce the same outcome.

After fixing the issue, the -s parameter will properly reflect the specified concurrency level, resulting in a noticeable difference in execution time.

For example:
Download the repository from https://github.com/vbuterin/blog, and then upload the site folder. After uploading all the files, perform the upload operation again. This second operation will only compare all files and will not trigger an actual upload. The time taken is as follows:

-s 15: 2.4 minutes,
-s 6: 4.7 minutes.
default is 6: 4.66 minutes.

@qzhodl
Copy link

qzhodl commented Aug 21, 2024

As a good PR practice, providing clear comments is crucial. You should not assume that the reviewer has the full context of the issue (in fact, DL does not have context for this issue). Therefore, it is essential to include:

  • A link to the reported issue
  • An analysis of the root cause
  • A description of how you tested the fix
  • The results of your testing

src/index.js Outdated Show resolved Hide resolved
src/utils/uploader.js Show resolved Hide resolved
@qzhodl
Copy link

qzhodl commented Aug 21, 2024

ethstorage/ethstorage-sdk#17 (comment) #15 (review)

Fix the bug where the threadPoolSize parameter has no effect. The issue is caused by the parameter being passed as a string, which fails to be recognized correctly. As a result, the concurrency level defaults to the maximum, causing -6 and -12 to produce the same outcome.

After fixing the issue, the -s parameter will properly reflect the specified concurrency level, resulting in a noticeable difference in execution time.

For example: Download the repository from https://github.com/vbuterin/blog, and then upload the site folder. After uploading all the files, perform the upload operation again. This second operation will only compare all files and will not trigger an actual upload. The time taken is as follows:

-s 15: 2.4 minutes, -s 6: 4.7 minutes.

I believe your test plan should at least cover the scenario described in the reported issue: ensuring that the time cost is the same when using the default threadPoolSize versus -s 6

@iteyelmp iteyelmp requested a review from qzhodl August 21, 2024 11:33
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.

2 participants