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

[Discussion]let IncreaseFullHistoryTsLow be a public API #9210

Closed
wolfkdy opened this issue Nov 24, 2021 · 3 comments
Closed

[Discussion]let IncreaseFullHistoryTsLow be a public API #9210

wolfkdy opened this issue Nov 24, 2021 · 3 comments

Comments

@wolfkdy
Copy link
Contributor

wolfkdy commented Nov 24, 2021

Currently, the fullHistoryTsLow is a member of CompactRangeOptions, that means a CF's fullHistoryTsLow is advanced only when users submit a CompactRange request.
However, users may want to advance the fllHistoryTsLow without an immediate compact, users may construct a fake range(which actually does not exist in the db, and submit a CompactRange task with the fake range. This should work but not neat.
So, @riversand963 , how about make IncreaseFullHistoryTsLow a public API so users can advance each CF's fullHistoryTsLow seperately?

@riversand963
Copy link
Contributor

@wolfkdy this sounds a reasonable ask. We avoided doing so in our first attempt because we wanted to defer the addition of public APIs. Also, the current implementation of increasing full_history_ts_low may have an edge case. If multiple threads try to increase the threshold at the same time, it will be increased to the max, but callers in different threads may not learn about the actual new value, as discussed in #7884 (comment).
I hope this can be resolved.
Would you like to contribute this feature?

@wolfkdy
Copy link
Contributor Author

wolfkdy commented Nov 25, 2021

Would you like to contribute this feature?

@riversand963 Thanks, we'll try to post an mr in a week.

I hope this can be resolved.

I think this can be resolved by also making the GetFullHistoryTsLow public, we can discuss about the details when the code is ready.

@sunlike-Lipo
Copy link
Contributor

sunlike-Lipo commented Dec 8, 2021

@riversand963
I repost a pull request for this discussion. #9221
Shall you take a look when free, thanks!

facebook-github-bot pushed a commit that referenced this issue Dec 23, 2021
Summary:
As (#9210) discussed, the **full_history_ts_low** is a member of CompactRangeOptions currently, which means a CF's fullHistoryTsLow is advanced only when users submit a CompactRange request.
However, users may want to advance the fllHistoryTsLow without an immediate compact.
This merge make IncreaseFullHistoryTsLow to a public API so users can advance each CF's fullHistoryTsLow seperately.

Pull Request resolved: #9221

Reviewed By: akankshamahajan15

Differential Revision: D33201106

Pulled By: riversand963

fbshipit-source-id: 9cb1d013ba93260f72e16353e693ffee167b47ee
@riversand963 riversand963 linked a pull request Dec 23, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants