feat: Add API to activate/deactivate clusters #95
Conversation
sbernauer
force-pushed
the
feat/activation-api
branch
from
edf21ee to
4fd0463
Compare
trino-lb/src/http_server/mod.rs
Outdated
| .route( | ||
| "/admin/activate-cluster/{cluster_name}", | ||
| post(admin::post_activate_cluster), | ||
| ) | ||
| .route( | ||
| "/admin/deactivate-cluster/{cluster_name}", | ||
| post(admin::post_deactivate_cluster), | ||
| ) | ||
| .route("/admin/cluster-status", get(admin::get_cluster_status)) |
There was a problem hiding this comment.
note: I think it makes sense to not even add these routes if no admin credentials are configured. This would also remove the need for the NoAdminAuthenticationMethodDefined error variant above.
There was a problem hiding this comment.
I prefer nice error handling.
Getting a 403 with a helpful error message is way nicer than a 404 and going what the heck.
There was a problem hiding this comment.
Yeah I guess that is a better user experience.
I just thought checking for if let Some(_) = admin_creds on every handler call is quite cumbersome.
There was a problem hiding this comment.
Ah I see, but
- it's only two endpoints
- The if let Some() would be needed in any case, as we need to check the username and password. We could do something with middleware and stuff but that sounds like premature optimization
| Error::SetClusterStateInPersistence { .. } => StatusCode::INTERNAL_SERVER_ERROR, | ||
| Error::GetAllClusterStates { .. } => StatusCode::INTERNAL_SERVER_ERROR, | ||
| }; | ||
| (status_code, format!("{self}")).into_response() |
There was a problem hiding this comment.
suggestion: Use self.to_string() to avoid unnecessary format! call (because to_string most likely already uses a format! call internally via the Display impl).
| (status_code, format!("{self}")).into_response() | |
| (status_code, self.to_string()).into_response() |
There was a problem hiding this comment.
I would prefer to keep it as is for consistency with all the other places.
Long term we should use a different formatter actually
There was a problem hiding this comment.
I would prefer to keep it as is for consistency with all the other places.
Sure, but that format! call is literally useless (it even has overhead). I think doing the better thing here trumps consistency.
Long term we should use a different formatter actually
Different formatter? Or do you mean different format? Otherwise I'm confused by this statement and would require a more detailed explanation.
| /// Get the status of the Trino clusters | ||
| #[instrument(name = "GET /admin/cluster-status", skip(state))] | ||
| pub async fn get_cluster_status( | ||
| State(state): State<Arc<AppState>>, | ||
| ) -> Result<Json<BTreeMap<TrinoClusterName, ClusterStats>>, Error> { |
There was a problem hiding this comment.
question: Does it make sense to also support retrieving the status of a single cluster via GET /admin/cluster-status/{cluster_name}?
There was a problem hiding this comment.
Sounds like a nice addition yes. Do you want me to do this in this PR already?
There was a problem hiding this comment.
Added in e43e373.
This also improves the consistency of the HTTP paths
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Co-authored-by: Techassi <[email protected]>
Co-authored-by: Techassi <[email protected]>
Co-authored-by: Techassi <[email protected]>
sbernauer
force-pushed
the
feat/activation-api
branch
from
042689f to
8cc8f97
Compare
Closes #1
New endpoints
Tested with and without autoscaling