-
Notifications
You must be signed in to change notification settings - Fork 133
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
[Storage] Replace Azure CLI with python SDK #638
Conversation
3ac153a
to
d137968
Compare
d137968
to
bc17fb5
Compare
Rebased on the recent master. |
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.
One tiny change request and a refactoring ticket suggestion, then we should be good for a merge.
medusa/storage/azure_storage.py
Outdated
name=object_key, | ||
data=data, | ||
overwrite=True, | ||
max_concurrency=4, |
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.
Todo: after doing some benchmarks, it looks like 16 gives us better performance. Could you set this value?
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.
Pushed a change.
max_workers=self.config.concurrent_transfers, | ||
multi_part_upload_threshold=int(self.config.multi_part_upload_threshold) | ||
) | ||
def _get_or_create_event_loop(self) -> asyncio.AbstractEventLoop: |
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.
Suggestion: this seems to be repeated in all the storage classes, maybe we could refactor it to have a single version that all storage classes will use? In the abstract storage possibly?
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.
It's indeed repeated, I'll be pushing a different PR fixing this.
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #638 +/- ##
==========================================
- Coverage 80.99% 80.97% -0.02%
==========================================
Files 55 53 -2
Lines 4666 4652 -14
Branches 623 661 +38
==========================================
- Hits 3779 3767 -12
+ Misses 861 857 -4
- Partials 26 28 +2
|
Fixes #628.
Fixes #637.