Skip to content

Commit

Permalink
refactor: Change product_version in the jvm mod to u16 for consistency
Browse files Browse the repository at this point in the history
  • Loading branch information
NickLarsenNZ committed Feb 12, 2025
1 parent d74ba65 commit 2640426
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
18 changes: 11 additions & 7 deletions rust/operator-binary/src/config/jvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub enum Error {
},

#[snafu(display("Trino version {version} is not yet supported, as the correct JVM configuration is not yet known"))]
TrinoVersionNotSupported { version: String },
TrinoVersionNotSupported { version: u16 },

#[snafu(display("failed to merge jvm argument overrides"))]
MergeJvmArgumentOverrides { source: role_utils::Error },
Expand All @@ -37,7 +37,7 @@ pub enum Error {
// Currently works for all supported versions (451 and 455 as of 2024-09-04) but maybe be changed
// in the future depending on the role and version.
pub fn jvm_config(
product_version: &str,
product_version: u16,
merged_config: &TrinoConfig,
role: &Role<TrinoConfigFragment, GenericRoleConfig, JavaCommonConfig>,
role_group: &str,
Expand Down Expand Up @@ -98,15 +98,15 @@ pub fn jvm_config(
/// This enables us to write version-independent tests, which don't need updating for every new
/// Trino version.
#[cfg(test)]
fn recommended_trino_jvm_args(_product_version: &str) -> Result<Vec<String>, Error> {
fn recommended_trino_jvm_args(_product_version: u16) -> Result<Vec<String>, Error> {
Ok(vec!["-RecommendedTrinoFlag".to_owned()])
}

#[cfg(not(test))]
fn recommended_trino_jvm_args(product_version: &str) -> Result<Vec<String>, Error> {
fn recommended_trino_jvm_args(product_version: u16) -> Result<Vec<String>, Error> {
match product_version {
// Copied from https://trino.io/docs/451/installation/deployment.html
"451" => Ok(vec![
451..455 => Ok(vec![
"-XX:InitialRAMPercentage=80".to_owned(),
"-XX:MaxRAMPercentage=80".to_owned(),
"-XX:G1HeapRegionSize=32M".to_owned(),
Expand All @@ -126,7 +126,7 @@ fn recommended_trino_jvm_args(product_version: &str) -> Result<Vec<String>, Erro
// Copied from:
// - https://trino.io/docs/455/installation/deployment.html#jvm-config
// - https://trino.io/docs/470/installation/deployment.html#jvm-config
"455" | "470" => Ok(vec![
455.. => Ok(vec![
"-XX:InitialRAMPercentage=80".to_owned(),
"-XX:MaxRAMPercentage=80".to_owned(),
"-XX:G1HeapRegionSize=32M".to_owned(),
Expand All @@ -151,6 +151,8 @@ fn recommended_trino_jvm_args(product_version: &str) -> Result<Vec<String>, Erro

#[cfg(test)]
mod tests {
use std::str::FromStr;

use indoc::indoc;
use stackable_trino_crd::{TrinoCluster, TrinoRole};

Expand Down Expand Up @@ -270,8 +272,10 @@ mod tests {
let merged_config = trino.merged_config(&role, &rolegroup_ref, &[]).unwrap();
let coordinators = trino.spec.coordinators.unwrap();

let product_version = trino.spec.image.product_version();

jvm_config(
trino.spec.image.product_version(),
u16::from_str(product_version).expect("trino version as u16"),
&merged_config,
&coordinators,
"default",
Expand Down
5 changes: 4 additions & 1 deletion rust/operator-binary/src/controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,11 @@ fn build_rolegroup_config_map(
) -> Result<ConfigMap> {
let mut cm_conf_data = BTreeMap::new();

let product_version = &resolved_product_image.product_version;
let product_version =
u16::from_str(product_version).context(ParseTrinoVersionSnafu { product_version })?;
let jvm_config = config::jvm::jvm_config(
&resolved_product_image.product_version,
product_version,
merged_config,
role,
&rolegroup_ref.role_group,
Expand Down

0 comments on commit 2640426

Please sign in to comment.