-
Notifications
You must be signed in to change notification settings - Fork 30
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
Allow configuring resource requirements for each pipeline #940
Conversation
Signed-off-by: Lalith Suresh <lalith@feldera.com>
Signed-off-by: Lalith Suresh <lalith@feldera.com>
0.1.1 has a transitive dependency on `ahash^0.4.4`, which got yanked: https://docs.rs/ahash/0.4.4/ahash/index.html Signed-off-by: Lalith Suresh <lalith@feldera.com>
@@ -167,3 +172,31 @@ pub struct FormatConfig { | |||
#[schema(value_type = Object)] | |||
pub config: YamlValue, | |||
} | |||
|
|||
#[derive(Debug, Clone, Eq, PartialEq, Default, Serialize, Deserialize, ToSchema)] | |||
pub struct ResourceConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about it, but I'd just make all these types here usize
and call it a day... if everything is the same type makes it easier to compare and do calculation on them, otherwise I have to start worrying about overflow when I compose them.
Or if you have cpu_min or max and want to use this this to index an array at some point it has to be usize again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are passed to certain external types in a cluster manager (at which point they actually becomes numeric strings with units). I didn't want to import those types here, hence the selection.
#[serde(default)] | ||
pub cpu_max: Option<u16>, | ||
|
||
/// The minimum memory in Megabytes to reserve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change all these to just bytes.. it's easier to remember that all units are always in unit types rather than having to remember which one is bytes, megabytes or gigabytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want the smallest unit of storage and memory to be 1MB, hence the choice here. There's little reason to go below that granularity, and it is also unergonomic in the API/UI.
I'll make the names explicit instead: memory_mb_min
, memory_mb_max
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assume we mean Mebibyte as opposed to Megabyte here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we mean mega (1000^2) not mebi (1024^2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want to use mebi? seems like the saner unit here :P
@@ -111,7 +111,8 @@ COPY sql-to-dbsp-compiler/lib database-stream-processor/sql-to-dbsp-compiler/lib | |||
COPY sql-to-dbsp-compiler/temp database-stream-processor/sql-to-dbsp-compiler/temp | |||
COPY sql-to-dbsp-compiler/SQL-compiler/sql-to-dbsp database-stream-processor/sql-to-dbsp-compiler/SQL-compiler/sql-to-dbsp | |||
# Run the precompile phase to speed up Rust compilations during deployment | |||
RUN ./compiler-server --compiler-working-directory=/working-dir --sql-compiler-home=/database-stream-processor/sql-to-dbsp-compiler --dbsp-override-path=/database-stream-processor --precompile | |||
# TODO: disabling this to keep compiler image sizes small | |||
# RUN ./compiler-server --compiler-working-directory=/working-dir --sql-compiler-home=/database-stream-processor/sql-to-dbsp-compiler --dbsp-override-path=/database-stream-processor --precompile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this accidentially committed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this image is only used in the cloud form factor where this helps keeps images small.
8f27329
to
dc3bf54
Compare
This patch extends the PipelineConfig API type to also specify resource requirements. For now, we support a min and max for CPU (in unit cores), memory (in MB) and storage (in MB). These are only meant to be used in Feldera Cloud and will not result in any resource reservations or enforcements in other supported form factors. Signed-off-by: Lalith Suresh <lalith@feldera.com>
Signed-off-by: Lalith Suresh <lalith@feldera.com>
dc3bf54
to
f0285d1
Compare
The PR also piggybacks a couple of separate self-contained commits.
Is this a user-visible change (yes/no): yes