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

container: add interval_set #17087

Merged
merged 1 commit into from
Mar 15, 2024
Merged

container: add interval_set #17087

merged 1 commit into from
Mar 15, 2024

Conversation

andrwng
Copy link
Contributor

@andrwng andrwng commented Mar 14, 2024

This adds an interval_set class, similar to interval_map with a couple key differences:

  • it is a set type, and only tracks the intervals rather than an additional value type
  • it supports inserting overlapping intervals by coalescing them

This will be a used in an upcoming change to track regions within a file to skip over, which will be key to a append-only log implementation that truncates by appending metadata to the log.

The structure and some of the implementation of the interval_set is copied directly from interval_map, though the insert() method is significantly different, given the above behavioral differences.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

Will review fully later, but initial thoughts:

  1. Should this go src/v/container if it isn't used in io:: and if it's going to be used by the new local-storage layer, then I think that is going to be separate from the io:: facilities.
  2. Since you are dealing with coalescing of interval overlaps, I wonder if you would want to use boost/icl? I think that the aggregation of overlaps there has some unique properties like favoring new/old inserts that overlap. But anyway, overall this looks good.

@andrwng
Copy link
Contributor Author

andrwng commented Mar 14, 2024

I wonder if you would want to use boost/icl?

Ah yea it wouldn't be a big lift to use that. Will consider for sure -- the only reason I can think of to have our own impl would be to control the underlying container, which isn't the biggest win.

@dotnwat
Copy link
Member

dotnwat commented Mar 14, 2024

I wonder if you would want to use boost/icl?

Ah yea it wouldn't be a big lift to use that. Will consider for sure -- the only reason I can think of to have our own impl would be to control the underlying container, which isn't the biggest win.

Having our own implementation is also nice because we don't have to learn another boost library.

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

looks solid to me. comments and location of file is i think the only substantive feedback

src/v/io/interval_set.h Outdated Show resolved Hide resolved
This adds an interval_set class, similar to interval_map with a couple
key differences:
- it is a set type, and only tracks the intervals rather than an
  additional value type
- it supports inserting overlapping intervals by coalescing them

This will be a used in an upcoming change to track regions within a file
to skip over, which will be key to a append-only log implementation that
truncates by appending metadata to the log.

The structure and some of the implementation of the interval_set is
copied directly from interval_map, though the insert() method is
significantly different, given the above behavioral differences.
@andrwng andrwng changed the title io: add interval_set container: add interval_set Mar 15, 2024
@andrwng
Copy link
Contributor Author

andrwng commented Mar 15, 2024

Should this go src/v/container if it isn't used in io:: and if it's going to be used by the new local-storage layer, then I think that is going to be separate from the io:: facilities.

Yeah that's a good point. Done

@andrwng andrwng merged commit 4224c1e into redpanda-data:dev Mar 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants