Separate Ingress from Worker role #2209
Conversation
tillrohrmann
force-pushed
the
issues/1312
branch
from
6e66256
to
1c15f70
Compare
There was a problem hiding this comment.
LGTM! I left a really minor comment regarding where to handle compatibility. I think (might be wrong) it will be cleaner if we automatically start the ingress role if the worker role is set and feature flag is not set. Instead of still manage ingress from inside the worker.
Then dropping the experimental feature will require change in only one file
crates/node/src/lib.rs
Outdated
| let ingress_role = if config | ||
| .ingress | ||
| .experimental_feature_enable_separate_ingress_role | ||
| && config.has_role(Role::Ingress) | ||
| { |
There was a problem hiding this comment.
Should the ingress role also be create if the feature is not set but this node has a Worker role ? so it's backward compatible ?
crates/worker/src/lib.rs
Outdated
| // todo remove once the safe fallback version supports the Ingress role | ||
| let ingress_http = if config | ||
| .ingress | ||
| .experimental_feature_enable_separate_ingress_role | ||
| { | ||
| None | ||
| } else { | ||
| // TODO sort this out! | ||
| // it's on https://github.com/restatedev/restate/pull/2172 | ||
| let partition_routing_refresher = | ||
| PartitionRoutingRefresher::new(metadata_store_client.clone()); | ||
|
|
||
| let rpc_router = ConnectionAwareRpcRouter::new(router_builder); | ||
| // http ingress | ||
| Some(HyperServerIngress::from_options( | ||
| &config.ingress, | ||
| RpcRequestDispatcher::new(PartitionProcessorRpcClient::new( | ||
| networking.clone(), | ||
| rpc_router, | ||
| metadata.updateable_partition_table(), | ||
| partition_routing_refresher.partition_routing(), | ||
| )), | ||
| metadata.updateable_schema(), | ||
| ingress_health_status, | ||
| )) | ||
| }; |
There was a problem hiding this comment.
I am wondering if this should be done in crates/node/src/lib.rs instead? By automatically starting the ingress Role the worker will not have to know/handle this case
There was a problem hiding this comment.
Yes, I think this makes a lot of sense. Thanks for catching this :-) I will update the PR accordingly.
crates/types/src/nodes_config.rs
Outdated
| @@ -48,6 +48,7 @@ pub enum Role { | |||
| MetadataStore, | |||
| /// [IN DEVELOPMENT] Serves a log server for replicated loglets | |||
| LogServer, | |||
| Ingress, | |||
There was a problem hiding this comment.
Maybe better to name this HttpIngress in case we ever split the Kafka ingress?
There was a problem hiding this comment.
Will update it.
This commit separates the ingress from the worker role. To support backward compatibility this feature is not yet turned on by default and users need to enable it explicity via `experimental-feature-enable-separate-ingress-role` to be able to configure the ingress role separately. When enabled, users can place the ingress role on a set of freely chosen nodes. This fixes restatedev#1312.
tillrohrmann
force-pushed
the
issues/1312
branch
from
1c15f70
to
2eafd58
Compare
I wonder if this is needed/worth doing at all. I expect no user setting up roles now anyway, because we never released the distributed runtime :) |
I wanted to make the behavior controllable. W/o the flag and just checking for either of the roles or both, the cluster behavior would change when going back from the next version to this version (assuming we check for either role, then we would start spawning ingresses where ever a worker role is running). W/ the flag, one can choose whether the old behavior (colocation-with workers) or the new ingress role is desirable. |
|
Ok my bad, you're right we need the flag, my understanding was this change is not a storage change, but it is because we write in the nodes configuration the roles. gonna do another pass now |
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) 2024 - Restate Software, Inc., Restate GmbH. | |||
| // Copyright (c) 2024-2024 - Restate Software, Inc., Restate GmbH. | |||
There was a problem hiding this comment.
| // Copyright (c) 2024-2024 - Restate Software, Inc., Restate GmbH. | |
| // Copyright (c) 2024-2024 = 0 - Restate Software, Inc., Restate GmbH. |
|
|
||
| type IngressHttp<T> = HyperServerIngress<Schema, RpcRequestDispatcher<T>>; | ||
|
|
||
| pub struct IngressRole<T> { |
There was a problem hiding this comment.
carrying this T around is soooo annoying and leaky 😢
| ingress_http: IngressHttp<T>, | ||
| } | ||
|
|
||
| impl<T: TransportConnect> IngressRole<T> { |
This commit separates the ingress from the worker role. To support backward compatibility this feature is not yet turned on by default and users need to enable it explicity via
experimental-feature-enable-separate-ingress-roleto be able to configure the ingress role separately. When enabled, users can place the ingress role on a set of freely chosen nodes.This fixes #1312.