bucket: use block.MetaFetcher to fetch blocks #1988
Conversation
khyatisoneji
force-pushed
the
1941-metaFetcher
branch
2 times, most recently
from
9c79cd2 to
50e6804
Compare
There was a problem hiding this comment.
Awesome, good work 👍 Generally ok, only style issues. I am suprised that there is not much to improve given your first contribution to Thanos 👍
Thanks, commented.
Also there is a CI error. You can click on red link to show the actual error at the bottom (: (tip: do make format)
| @@ -406,25 +412,17 @@ func refresh(ctx context.Context, logger log.Logger, bucketUI *ui.Bucket, durati | |||
| }) | |||
| } | |||
|
|
|||
| func download(ctx context.Context, logger log.Logger, bkt objstore.Bucket) (blocks []metadata.Meta, err error) { | |||
| func download(ctx context.Context, logger log.Logger, bkt objstore.Bucket, fetcher *block.MetaFetcher) (blocks []metadata.Meta, err error) { | |||
There was a problem hiding this comment.
I think we can return map here and operate on map on download caller to avoid unnecessary allocations/operations. The reason is rather readability not performance.
There was a problem hiding this comment.
the caller function is marshaling the returned array, is it ok if we return map instead of array now?
Will there be any change in the caller because of this?
There was a problem hiding this comment.
In this case probably no (: ignore my comment, you are right 👍
khyatisoneji
force-pushed
the
1941-metaFetcher
branch
2 times, most recently
from
df5e846 to
2c291ee
Compare
|
As a last thing: Can you test manually? e.g |
Signed-off-by: khyatisoneji <[email protected]>
khyatisoneji
force-pushed
the
1941-metaFetcher
branch
from
2c291ee to
fd895dd
Compare
|
|
|
Thank you! Good job 🥇 |
Issue link: #1941
Changes
use
block.metaFetcherinbucketcommand