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.

Sign up for GitHub

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

Jump to bottom

Added tools bucket marker for no-downsample.json #5945

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Added tools bucket marker for no-downsample.json #5945

merged 1 commit into from
Dec 15, 2022

Conversation

RohitKochhar
Copy link
Contributor

@RohitKochhar RohitKochhar commented Dec 7, 2022 •
edited
Loading

Signed-off-by: Rohit Singh [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

@RohitKochhar
Copy link
Contributor Author

Linked Issue: #5944

@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch from c9f4934 to b35d7ec Compare December 7, 2022 18:17
yeya24
yeya24 reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

I think we still need to update the downsample code to exclude blocks that have the marker.

pkg/block/block.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 7, 2022
@RohitKochhar
Copy link
Contributor Author

@yeya24 I have added in the code that filters the block with that filter. Just split it into two commits to reflect adding the flag to the command itself and then another that reflected adding the logic.

@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch from 204452c to 5fe8343 Compare December 7, 2022 18:53
@RohitKochhar RohitKochhar changed the title Added tools bucket marker flag handling for no-downsample.json Added tools bucket marker for no-downsample.json Dec 7, 2022
@RohitKochhar RohitKochhar marked this pull request as ready for review December 8, 2022 15:25
@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch 2 times, most recently from c9c504c to f81ed16 Compare December 8, 2022 16:02
yeya24
yeya24 reviewed Dec 9, 2022
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks! Generally this makes sense to me. Can we add some tests?

@RohitKochhar
Copy link
Contributor Author

Sounds good, I'll get working on that over the next day. Do you have any insight why the PR is failing CI? I looked through the action results, but can't diagnose the issue.

@yeya24
Copy link
Contributor

yeya24 commented Dec 9, 2022

Do you have any insight why the PR is failing CI? I looked through the action results, but can't diagnose the issue.

This one is a flake I believe. Just retried the workflow and let's see if it works this time.

@RohitKochhar
Copy link
Contributor Author

@yeya24, adding tests now. I'm wondering what you think would be the best ways to test this. My current idea is to test the no-downsample filter and mark creation, similar to TestIgnoreDeletionMarkFilter_Filter in fetcher_test.go#1073, but besides for that I don't think that there is much to test here

@yeya24
Copy link
Contributor

yeya24 commented Dec 11, 2022

My current idea is to test the no-downsample filter and mark creation, similar to TestIgnoreDeletionMarkFilter_Filter in fetcher_test.go#1073

Yeah having this coverage is good for now. Thanks for working on this!

@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch 2 times, most recently from ab85ef6 to 3ec072a Compare December 12, 2022 16:25
@RohitKochhar
Copy link
Contributor Author

@yeya24 added the test in a similar format to how no-compact marks are testing, looks like it's passing CI so I think this might be good!

yeya24
yeya24 reviewed Dec 12, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch from 3ec072a to 9f757e9 Compare December 12, 2022 17:43
@RohitKochhar
Copy link
Contributor Author

@yeya24 Fixed the changelog entry

@RohitKochhar RohitKochhar mentioned this pull request Dec 12, 2022
Open
@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch 2 times, most recently from a3568ca to fe2a5df Compare December 13, 2022 13:44
@RohitKochhar RohitKochhar mentioned this pull request Dec 13, 2022
Open
@RohitKochhar
Copy link
Contributor Author

@fpetkovski @yeya24 Looking for an update on this... curious why the CI checks seem to have hung...

@yeya24
Copy link
Contributor

yeya24 commented Dec 14, 2022 •
edited
Loading

@RohitKochhar The changelog is not fixed... Please also mention we will skip downsampling for blocks with the new marker.
Have no idea about the CI, but I guess pushing another commit will retrigger it.

@RohitKochhar RohitKochhar force-pushed the add_no_downsample_flag branch from fe2a5df to 8a6bdf7 Compare December 14, 2022 19:37
@RohitKochhar
Copy link
Contributor Author

@yeya24 thanks for your quick response, I added the additional information in the change log. doing another push seems to have properly retriggered CI as well.

yeya24
yeya24 approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution! @RohitKochhar

@RohitKochhar
Copy link
Contributor Author

Perfect, does anything else have to happen to get this merged?

@yeya24 yeya24 merged commit 15415fb into thanos-io:main Dec 15, 2022
fpetkovski pushed a commit to fpetkovski/thanos that referenced this pull request Jan 13, 2023
@RohitKochhar @rabenhorst
Added tools bucket marker flag to mark block for no-downsample (thano…
67ba676 
…s-io#5945)

Signed-off-by: Rohit Singh <[email protected]>

Signed-off-by: Rohit Singh <[email protected]>
@RohitKochhar RohitKochhar mentioned this pull request Feb 27, 2023
Open
@yangfan-njupt yangfan-njupt mentioned this pull request Mar 2, 2023
Closed
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request May 18, 2023
@RohitKochhar
Added tools bucket marker flag to mark block for no-downsample (thano…
17434f0 
…s-io#5945)

Signed-off-by: Rohit Singh <[email protected]>

Signed-off-by: Rohit Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski left review comments

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@yeya24 yeya24 yeya24 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RohitKochhar @yeya24 @fpetkovski