Skip to content

Conversation

@congminh1254
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Feb 13, 2023

Pull Request Test Coverage Report for Build 4322289600

  • 42 of 43 (97.67%) changed or added relevant lines in 2 files are covered.
  • 13 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 93.527%

Changes Missing Coverage Covered Lines Changed/Added Lines %
boxsdk/util/chunked_uploader.py 41 42 97.62%
Files with Coverage Reduction New Missed Lines %
boxsdk/object/retention_policy.py 2 78.13%
boxsdk/client/client.py 11 86.14%
Totals Coverage Status
Change from base Build 4113948266: 0.05%
Covered Lines: 3439
Relevant Lines: 3677

💛 - Coveralls

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mwwoda mwwoda left a comment

Choose a reason for hiding this comment

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

Couple of nits

Comment on lines 272 to 277
value = upload_results.pop(0)
if isinstance(value, Exception):
time.sleep(randint(0, 1))
raise value
time.sleep(randint(0, 1))
return value
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move sleep before isinstance check and get rid of duplicated sleep call

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure I was testing different delays for exc and actual response, but forgot to move it as I use now same value.

for future in as_completed(futures):
future.result()
except Exception as exc:
self._executor.shutdown(wait=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like by default wait is set to True - https://docs.python.org/3/library/concurrent.futures.html
You could get rid of it if you want

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but I want to make it clear that we wait

Copy link
Contributor

@mwwoda mwwoda left a comment

Choose a reason for hiding this comment

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

🚀

@lukaszsocha2 lukaszsocha2 merged commit 506ce0d into main Mar 3, 2023
@lukaszsocha2 lukaszsocha2 deleted the chunk-upload-multithreading branch March 3, 2023 10:36
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.

6 participants