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

[Boltdb-shipper] If S3 ListObjects returns the directory itself getDBNameFromObjectKey fails #3173

Closed
wants to merge 2 commits into from

Conversation

netdata-be
Copy link

Chubaofs returns the following:

$ s3cmd ls s3://loki/index/index_18641/
2021-01-14 08:46            0  s3://loki/index/index_18641/
2021-01-14 08:46         3952  s3://loki/index/index_18641/xxx-1610612684957170124-1610613044.gz
2021-01-14 09:01         2990  s3://loki/index/index_18641/xxx-1610612684957170124-1610613900.gz
2021-01-14 09:16         6552  s3://loki/index/index_18641/xxx-1610612684957170124-1610614800.gz
2021-01-14 09:31         9487  s3://loki/index/index_18641/xxx-1610612684957170124-1610615700.gz

So the directory itself is also returned, therefore we need to skip if the object ends with a /

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ slim-bean
❌ Wouter D'Haeseleer


Wouter D'Haeseleer seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-io
Copy link

codecov-io commented Jan 14, 2021

Codecov Report

Merging #3173 (ea7f637) into master (322e4bc) will increase coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3173   +/-   ##
=======================================
  Coverage   63.25%   63.25%           
=======================================
  Files         188      188           
  Lines       16304    16316   +12     
=======================================
+ Hits        10313    10321    +8     
- Misses       5051     5055    +4     
  Partials      940      940           
Impacted Files Coverage Δ
pkg/storage/stores/shipper/compactor/table.go 60.81% <0.00%> (-0.84%) ⬇️
pkg/storage/stores/shipper/downloads/table.go 65.57% <50.00%> (-0.26%) ⬇️
pkg/storage/stores/shipper/util/util.go 40.00% <100.00%> (+5.62%) ⬆️
pkg/canary/comparator/comparator.go 76.36% <0.00%> (-1.82%) ⬇️
pkg/logql/evaluator.go 90.23% <0.00%> (+0.35%) ⬆️
pkg/querier/queryrange/downstreamer.go 97.64% <0.00%> (+2.35%) ⬆️

If that's the case the getDBNameFromObjectKey will fail to split the names based on `/`
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Thanks for this PR -- we definitely don't want this failing on a legitimate use case.

@sandeepsukhani Can you take a look? All looks functionally to me, but I think there might be a better way to validate these than checking the suffix in four places.

@sandeepsukhani
Copy link
Contributor

Thanks for this PR -- we definitely don't want this failing on a legitimate use case.

@sandeepsukhani Can you take a look? All looks functionally to me, but I think there might be a better way to validate these than checking the suffix in four places.

I agree with Owen, we should move it to a common place. Maybe add a helper function which would accept a list of objects as inputs and return a list of objects after removing objects which end with a /. It makes future enhancement and writing tests easier.

@netdata-be
Copy link
Author

Thanks for this PR -- we definitely don't want this failing on a legitimate use case.
@sandeepsukhani Can you take a look? All looks functionally to me, but I think there might be a better way to validate these than checking the suffix in four places.

I agree with Owen, we should move it to a common place. Maybe add a helper function which would accept a list of objects as inputs and return a list of objects after removing objects which end with a /. It makes future enhancement and writing tests easier.

Would you mind doing that for me? I'm not a hero in this ;-)

@sandeepsukhani sandeepsukhani self-assigned this Jan 25, 2021
@pull-request-size pull-request-size bot added size/L and removed size/S labels Feb 26, 2021
@slim-bean
Copy link
Collaborator

I create a separate issue with these changes #3394

After pushing code to address the reviews I realized the CLA wasn't signed (likely because the email on the original commit didn't match the email on the github account) and another change also introduced a merge conflict I decided to make a separate so I can get this merged today and it should make the next release.

Apologies @netdata-be, not trying to take any credit away from your finding and fixing this issue, I hope you understand!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants