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

Fix ooo compacted blocks shipping #5416

Closed
wants to merge 4 commits into from
Closed

Fix ooo compacted blocks shipping #5416

wants to merge 4 commits into from

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jun 20, 2023 •
edited
Loading

What this PR does:

If OOO is enabled, shipper needs to upload compacted blocks.
Another thing is that we supports hot reload of OOO time window so we might need to support hot reload shipper configuration as well.

Which issue(s) this PR fixes:
Fixes #5402

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@yeya24
Copy link
Contributor Author

yeya24 commented Jun 21, 2023

Another thing is that we supports hot reload of OOO time window so we might need to support hot reload shipper configuration as well.

Maybe we should make that configuration as a function so that we can hot reload at runtime

@alanprot
Copy link
Member

Thanks. LGTM

@yeya24
Copy link
Contributor Author

yeya24 commented Jul 12, 2023

Another thing is that we supports hot reload of OOO time window so we might need to support hot reload shipper configuration as well.

I will try to fix this hot reload issue before I merge this pr.

@yeya24 yeya24 mentioned this pull request Jul 12, 2023
Merged
2 tasks
@yeya24
Copy link
Contributor Author

yeya24 commented Jul 12, 2023

Opened thanos-io/thanos#6526 as well

@yeya24 yeya24 force-pushed the fix-ooo-shipping branch from 38db4e9 to bf01afd Compare July 24, 2023 07:44
@yeya24 yeya24 mentioned this pull request Aug 3, 2023
Closed
@yeya24
Copy link
Contributor Author

yeya24 commented Aug 9, 2023

I am going to resolve the conflicts and get the pr merged.
And now the shipper can dynamically load compacted configuration so I think it should solve the issue.

@yeya24
fix ooo time window
0f0a0cf 
Signed-off-by: Ben Ye <[email protected]>
@yeya24
Copy link
Contributor Author

yeya24 commented Oct 29, 2023 •
edited
Loading

I just realized that OOO blocks or compacted blocks can already be uploaded to objstore when OOO time window is > 0 for a user.
The change in this pr is to enable dynamically turning on/off shipping compacted blocks based on OOO configuration. Since OOO configuration is a runtime config and can be turned on/off.

#5402 (comment)

@yeya24
add unit test
3d9f22d 
Signed-off-by: Ben Ye <[email protected]>
@yeya24
update changelog
8da37ff 
Signed-off-by: Ben Ye <[email protected]>
@yeya24
Copy link
Contributor Author

yeya24 commented Oct 29, 2023 •
edited
Loading

The change in this pr is to enable dynamically turning on/off shipping compacted blocks based on OOO configuration. Since OOO configuration is a runtime config and can be turned on/off.

I feel there is still an edge case here. A user has OOO enabled before but disabled OOO after. This might cause compacted OOO blocks unable to be uploaded because shipper turned uploading compacted blocks off.

I don't have a good plan on how to solve it. Maybe we can always turn on uploading compacted blocks. If we can turn it on for OOO and it doesn't impact anything. I don't why we cannot turn on it for normal case.

Update: Added #5625 to supercede this pr.

@yeya24 yeya24 mentioned this pull request Oct 29, 2023
Merged
3 tasks
@yeya24
Copy link
Contributor Author

yeya24 commented Oct 31, 2023

Superceded by #5625

@yeya24 yeya24 closed this Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@friedrich-at-adobe friedrich-at-adobe friedrich-at-adobe approved these changes

@alanprot alanprot alanprot approved these changes

@friedrichg friedrichg friedrichg approved these changes

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

Successfully merging this pull request may close these issues.

Unshipped blocks when out of order writes are enabled
4 participants
@yeya24 @alanprot @friedrichg @friedrich-at-adobe