feat(actix): New Register [INGEST-1494] #1421
Conversation
tobias-wilfert
force-pushed
the
tobias-wilfert/new-register
branch
from
0fcb4d2 to
d15a3b6
Compare
|
lgtm |
| @@ -36,7 +33,7 @@ impl Healthcheck { | |||
| /// | |||
| /// Panics if the service was not started using [`Healthcheck::start`] prior to this being used. | |||
| pub fn from_registry() -> Addr<Self> { | |||
| ADDRESS.read().as_ref().unwrap().clone() | |||
| REGISTRY.get().unwrap().healthcheck.clone() | |||
There was a problem hiding this comment.
I wonder if we could make this method generic on Service somehow. In C++ you could do std::get<T> to access a tuple by type, in Rust I would not know how to do it.
There was a problem hiding this comment.
yeah we wondered this as well but could not figure it out without dynamic dispatch (TypeId) but that would slow this down. I'm not sure how to do this at compile time, but it so feels like this should be possible.
tobias-wilfert
changed the title
|
|
||
| use relay_aws_extension::AwsExtension; | ||
| use relay_config::Config; | ||
| use relay_metrics::Aggregator; | ||
| use relay_redis::RedisPool; | ||
| use relay_system::Addr; |
There was a problem hiding this comment.
Please merge this input with the one below.
| @@ -26,6 +28,8 @@ use crate::middlewares::{ | |||
| }; | |||
| use crate::{endpoints, utils}; | |||
|
|
|||
| pub static REGISTRY: OnceBox<Registry> = OnceBox::new(); | |||
There was a problem hiding this comment.
Please make this non-public and hide the implementation detail that this uses a OnceBox. Especially, this is considered infallible, and the unwrap should be hidden behind an accessor method:
impl Registry {
fn get() -> &Self {
// some comment on why we think this is safe (once it is safe)
REGISTRY.get().unwrap()
}
}| @@ -105,6 +109,11 @@ impl From<Context<ServerErrorKind>> for ServerError { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| pub struct Registry { | |||
There was a problem hiding this comment.
Please add documentation to this type.
| @@ -36,7 +33,7 @@ impl Healthcheck { | |||
| /// | |||
| /// Panics if the service was not started using [`Healthcheck::start`] prior to this being used. | |||
| pub fn from_registry() -> Addr<Self> { | |||
There was a problem hiding this comment.
These static from_registry functions were a workaround to replace the trait method on actix::SystemService. Now that all our services are expected to be in the registry, we could move this to a mandatory Service::from_registry trait method which enforces that this is implemented for every service in the same way. Also, this means that we no longer have to copy-paste the doc comment.
Alternatively, we might not even need from_registry anymore. It's now as simple as:
Registry::get().healthcheck.send(_);There was a problem hiding this comment.
Having a ::from_registry() forces you to import the service type, this variation makes everyone import the Registry. The only argument I'm able to come up is that importing the service type makes it easier to discover in the code where the service is being used, so that would argue for keeping ::from_regsitry(). +1 to making it a trait method though.
General
With the generic
Addrin place since #1405 this PRmakes a new Register for the Services, which replaces
the current hardcoded solution.
Design choices
Arc<Option<Register>>however, this didn't work as
Arc::newis notconst fn.To avoid this
lazy_staticwas used however that inturn didn't work as
DerefMutwas required andlazy_staticonly implementsDeref.OnceBoxis currently used for the Register which isinitialised at the end of
ServiceState::start()thisavoids the need for any
Optionsbut also could causeissues in the future as the current
registryis createdat the beginning of the function and then gradually populated.
A similar approach might need to be used in case of issues.
Future Steps
Registerout of the way the next actor can be upgraded.A good choice for that is the
UpstreamActor.