Conversation
There was a problem hiding this comment.
En general lo veo super bien!
Creo que los dos puntos principales que tengo son:
-
Agregar una llave de particion a las tablas y agregar mas parametros para la creacion.
-
Organizar los schemas de las tablas en un archivo aparte. Así sera facil consultar como se ve la estructura de las tablas.
-
Utilizar Enums en vez de Strings donde sea posible. Esto nos ayudara a optimizar espacio y hacer las consultas mas rapidas.
Le pedi a gemini que me buscara esos types que podriamos tener como un dictionary type y me dio estos.
| Table | Field | Current Type | Recommended Enum Source / Variants |
|---|---|---|---|
| Deployments | restart_policy |
String |
Maps to crate::service::file::Restart (Always, No, OnFailure, UnlessStopped). |
| Deployments | trigger_type |
String |
New TriggerType enum: startup, cron, image_update, manual_reload. |
| Status | state |
String |
Based on bollard::config::ContainerStateStatusEnum, extended with not_found and unknown. |
| Status | health_status |
String |
Based on bollard::config::HealthStatusEnum (healthy, unhealthy, starting, none). |
src/service/instance.rs
Outdated
|
|
||
| impl ServiceInstance { | ||
| pub async fn run_container(&self) -> Result<(), ServiceConfigError> { | ||
| pub async fn run_container(&self, trigger_type: &str) -> Result<(), ServiceConfigError> { |
There was a problem hiding this comment.
Creo que trigger_type debería ser un Enum. Esto haría un poco mas robusta la implementación, evitaría la posibilidad de equivocarnos en el futuro.
| .and_then(|s| s.health.as_ref()) | ||
| .and_then(|h| h.status.map(|st| st.to_string())) | ||
| .unwrap_or_else(|| "none".to_string()); | ||
| let exit_code = state.and_then(|s| s.exit_code).map(|c| c as i32); |
There was a problem hiding this comment.
Veo que estos dos campos, tanto health como status son enums.
https://docs.rs/bollard/latest/bollard/config/enum.HealthStatusEnum.html
https://docs.rs/bollard/latest/bollard/config/enum.ContainerStateStatusEnum.html
Dada la naturaleza de estos Enums, creo que seria interesante utilizar estos enums directamente. (O convertir los enums a un enum propio interno de Dispenser).
Esto lo digo por que creo que seria beneficioso escribir estas columnas a las tablas en forma de Enum directamente (en Arrow esto seria un Dictorionary type o en Categorical / Enum en polars)
- https://arrow.apache.org/docs/python/data.html#dictionary-arrays
- https://docs.pola.rs/user-guide/expressions/categorical-data-and-enums/
Que opinas?
src/service/instance.rs
Outdated
| telemetry.track_status( | ||
| self.config.service.name.clone(), | ||
| String::new(), | ||
| "not_found".to_string(), |
There was a problem hiding this comment.
Creo que si vamos a necesitar nuestro propio enum para incluir estos variantes como NotFound
There was a problem hiding this comment.
Creo que para simplificar esto deberiamos tener un archivo llamado schema.rs donde definimos los schemas.
Tambien pienso que todos lo que es un enum dentro de dispenser deberiamos escribirlo como un dictionary type.
Por ejemplo.
Lines 238 to 253 in 1756744
Esto deberia caber en un byte, mientras que un string siempre va a consumir un byte por character.
Ahora le hago un pregunta a gemini para que consolide todos estos tipos.
| ) -> Result<(), DeltaTableError> { | ||
| let table = match deltalake::open_table(table_uri).await { | ||
| Ok(table) => table, | ||
| Err(DeltaTableError::NotATable(_)) => { |
There was a problem hiding this comment.
Creo que aqui al crear la tabla necesitamos agregar una llave de particion. Creo que en este caso seria por date. Creo que en este caso seria por date.
There was a problem hiding this comment.
Este es un ejemplo de una tabla que creamos para otro servicio de este estilo.
En el ejemplo de abajo hay algunos parametros importantes que creo que tenemos que tomar encuenta.
No digo que tengan que ser asi tal cual, de hecho creo que para este caso AutoOptimizeAutoCompact, AutoOptimizeOptimizeWrite, CheckpointInterval no son tan importantes. Pero puede ser que algunos de los otros sean muy relevantes.
Si te recomiendo 100% crear una funcion de este estilo para tener un lugar para el schema y la creacion de la tabla. Asi esta seria nuestro fuente de la verdad de como se ve la tabla y como se comporta. Esto pienso que iria en ese schema.rs
pub async fn create_table_if_not_exists(
location: &str,
) -> Result<DeltaTable, deltalake::DeltaTableError> {
log::info!("create_table_if_not_exists: checking location {}", location);
let columns = [
StructField::new("date", DataType::DATE, false),
StructField::new("timestamp", DataType::TIMESTAMP, false),
StructField::new("server_id", DataType::STRING, false),
StructField::new("event_id", DataType::STRING, false),
StructField::new("script", DataType::STRING, false),
StructField::new("message", DataType::STRING, true),
];
let result = CreateBuilder::new()
.with_location(location)
.with_columns(columns)
.with_partition_columns(["date"])
.with_save_mode(SaveMode::Ignore)
.with_configuration_property(TableProperty::DeletedFileRetentionDuration, Some("1d"))
.with_configuration_property(TableProperty::LogRetentionDuration, Some("6h"))
.with_configuration_property(TableProperty::AutoOptimizeAutoCompact, Some("true"))
.with_configuration_property(TableProperty::AutoOptimizeOptimizeWrite, Some("true"))
.with_configuration_property(TableProperty::CheckpointInterval, Some("20"))
.with_configuration_property(TableProperty::TargetFileSize, Some("128MB"))
.with_configuration_property(
TableProperty::DataSkippingStatsColumns,
Some("timestamp,server_id,script,event_id"),
)
.await;
match &result {
Ok(_) => log::info!("create_table_if_not_exists: success"),
Err(e) => log::error!("create_table_if_not_exists: failed: {}", e),
}
result
}
No description provided.