Skip to content

Make segment cache configurable #320

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
Closed
wants to merge 23 commits into from
Closed

Make segment cache configurable #320

wants to merge 23 commits into from

Conversation

razvan
Copy link
Member

@razvan razvan commented Oct 17, 2022 •
edited
Loading

@razvan razvan linked an issue Oct 17, 2022 that may be closed by this pull request
Make segment-cache size configurable and use emptyDir for it #306
Closed
3 tasks
@razvan razvan marked this pull request as ready for review October 18, 2022 14:33
@razvan razvan requested a review from a team October 18, 2022 14:34
@sbernauer
Copy link
Member

sbernauer commented Oct 20, 2022 •
edited
Loading

Some things that came up to my mind so far:

  • We only want to configure the segment cache for historicals (and therefore only want users the ability to configure it on the historicals role)
  • As this is a resource thing maybe we can add it to the resources struct to be consistent
  • If i say 100 GB i would have expected the emptyDir to have 100GB with e.g. 95GB usable space. This aligns with our other resource handling on the SDP

@razvan
Copy link
Member Author

razvan commented Oct 26, 2022

@sbernauer wrote:

We only want to configure the segment cache for historicals (and therefore only want users the ability to configure it on the historicals role)

This is now done. It required replacing DruidConfig with role specific configuration structs (HistoricalConfig, BrokerConfig, etc.) which made the size of this PR explode.

@razvan
Copy link
Member Author

razvan commented Oct 26, 2022

As this is a resource thing maybe we can add it to the resources struct to be consistent

I'm not convinced this is a "resource thing". The fact that we use an emptyDir to store the segments is an implementation detail IMO. It's not clear to me why we consider volumes as "resources" and mix them up memory and cpu.

If i say 100 GB i would have expected the emptyDir to have 100GB with e.g. 95GB usable space. This aligns with our other resource handling on the SDP

I disagree with your first sentence but you are right, this implementation goes against the current practice. I argued in the arch meeting of 26.10 that the current practice is wrong.

@fhennig fhennig self-assigned this Nov 1, 2022
fhennig
fhennig suggested changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@fhennig fhennig left a comment

Choose a reason for hiding this comment

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

Tests look nice!

From the issue

segment-cache size configurable. We default to 5% free percentage

you've used 10%, not sure how important this is

Thanks for refactoring the Druid config into 5 different structs!

docs/modules/ROOT/pages/usage.adoc
max: "4"
memory:
limit: '2Gi'
storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

This CRD change should be discussed in the Architecture meeting

rust/crd/src/storage.rs
pub fn update_container(&self, pb: &mut PodBuilder, cb: &mut ContainerBuilder) {
let volume_size = match self.segment_cache_size_gb {
Some(v) => v,
_ => 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not happy about this default value just being here in the function. especially since the 900m in the product config depends on this value. I don't have an idea what to change though, so there's no need to change anything.

razvan and others added 3 commits November 1, 2022 16:07
@fhennig fhennig removed their assignment Nov 2, 2022
@razvan razvan mentioned this pull request Nov 3, 2022
Closed
@razvan
Copy link
Member Author

razvan commented Nov 3, 2022

Decided to split this PR into two and close it.

The first PR that implements role based configuration is here: #332

The second PR that implements the actual functionality required here will depend on a new release of the operator framework and will follow.

@razvan razvan closed this Nov 3, 2022
@razvan razvan deleted the 306-make-segment-cache-size-configurable-and-use-emptydir-for-it branch November 3, 2022 16:33
bors bot pushed a commit that referenced this pull request Nov 4, 2022
@razvan @sbernauer
role based configuration (#332)
6c28e73 
Part of: #306 

This PR has been extracted from #320 which will be closed. The part that was left out is the actual configuration the of segment cache size. That will be implemented in a future PR and will require a new operator-rs release.

:green_circle: CI https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/druid-operator-it-custom/34/


Co-authored-by: Sebastian Bernauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhennig fhennig fhennig requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make segment-cache size configurable and use emptyDir for it
3 participants
@razvan @sbernauer @fhennig