-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
objstore/swift: use ncw/swift and support large files for OpenStack Swift #2732
Conversation
4eeb649
to
b8abc7c
Compare
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.
I love it, thanks to this. Minor suggestions only.
The only concern for SWIFT is that it's not strongly consistent, are you aware of this? This makes compaction tricky?
thanks for the swift review 😁 I'd like to wait some time to test it out more to be sure it works fine. Regarding the consistency issues. Am I missing out some other cases where it would could blow up? |
15833f8
to
a50af4a
Compare
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.
Some minor initial comments, I haven't finished looking through the code. Thanks a lot for this. How did your testing of this go BTW?
Thanks for another review! :) I'm fairly confident the uploading/reading/listing etc are working OK. |
I'll rebase once #2838 to verify the tests still passes even after the refactor |
38bfcce
to
a767f6d
Compare
I finally found a while to take a look at the failing tests. Now it's all green so hopefully everything is OK 🤞 I was afraid to merge this right away but it has been a while and it looks like there are no other active users of Swift so I'd go ahead and merge this if you are not against it? |
Rebased once again ♻️ |
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.
Sorry for the delay in the review. Some questions/nits in line.
5636947
to
e494718
Compare
@GiedriusS Thanks for going through it in detail. All comments were valid and should be fixed now. Tests related to Swift are passing but other seems to be flaky probably? 😕 PTAL |
@GiedriusS do you think we could move on with this? I'll rebase |
We're bumping into the same issue. We can offer to test on our QA envs, but there we might not have big enough buckets to 'need' the splitting up on uploads. |
6597776
to
a42626a
Compare
@bwplotka @GiedriusS PTAL, should be up to date again |
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Co-authored-by: Giedrius Statkevičius <giedriuswork@gmail.com> Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
a42626a
to
cb577fe
Compare
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
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.
The code looks good but I'm kind of hesitant to merge this because the new library doesn't support context.Context
and that means we have to add yet another timeout parameter. @FUSAKLA perhaps you know if there are any plans to add support for contexts to the library?
@GiedriusS Yep, that is not great but the current state of the swift integration is almost unusable because of the file size limitation. I opened an issue on the library and it showed up it's already considered (it was originally created before Google context library was created :)) ) so I'd send a PR there to add it. Considering the current status of the Swift integration and how long is this PR pending, I'd be glad to merge this and ASAP send a next PR to add the context support once done in the library if you wouldn't mind. Swift is still marked as experimental in the docs so maybe the lack of context is acceptable and still having the global timeout configuration? See the issues: |
Happy with this approach given the inusability of previous integration. Thanks for this work and sorry for lag in review. Good work! |
Resolves #2678
Closes #2665
As described in the issue, switch to new library to communicate with swift which supports large file types.
I also changed the maintainer of the swift integration to me since if this would be accepted, there would be not much of the old codebase left so it felt fair not to leave the burden on @sudhi-vm.
It supports also working with the large objects which was limit of the previous library.
Changes
Verification
All tests are passing, tried running it as a sidecar, compactor and store for now on real workload.
Works well but will continue testing it with higher load and bigger blocks.
Eventually would like to migrate production on it.