Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(actix): Update Store Actor [INGEST-1565] #1415

Merged
merged 17 commits into from
Aug 18, 2022

Conversation

tobias-wilfert
Copy link
Member

@tobias-wilfert tobias-wilfert commented Aug 16, 2022

General

This PR updates the Store service to use the generic Addr introduced
in #1405, to avoid duplicate code, it moves the shared code out
to its own module in relay-system called service.

Some minor changes have been made to address the comments of Jan
on #1397.

Future Steps

  • With the generic Addr in place the time has come to make a new
    registry for the services, that replaces the hardcoded current solution.

@tobias-wilfert tobias-wilfert force-pushed the tobias-wilfert/future-proofing-relay branch from 36d98ae to 6ca4754 Compare August 16, 2022 12:08
@tobias-wilfert tobias-wilfert changed the base branch from master to flub/generic-addr-nobox August 16, 2022 12:08
@tobias-wilfert tobias-wilfert changed the title Tobias wilfert/future proofing relay ref(actix): Update Store Actor Aug 16, 2022
Base automatically changed from flub/generic-addr-nobox to master August 16, 2022 13:24
@tobias-wilfert tobias-wilfert force-pushed the tobias-wilfert/future-proofing-relay branch from b9ab1f7 to 730621f Compare August 16, 2022 13:47
@flub
Copy link
Contributor

flub commented Aug 16, 2022

  • The struct StoreEnvelope has been renamed to StoreMessage to
    free up the StoreEnvelope name for an enum to keep the naming
    across Services consistent, since the corresponding enum in the
    Healthcheck service is named HealthcheckEnvelope.

I think this might have been the wrong call, StoreEnvelope is the existing message type of this actor, and we don't really want to go change these. It was a goal the keep our messages the same. It is also a message which instructs to "store an envelope", StoreMessage does kind of twist around the naming scheme.

Instead I'd call the envelope something like StoreMessageEnvelope or so, and for consistency you may as well rename HealthcheckEnvelope to HealthCheckMessageEnvelope.

It's perhaps a bit unfortunately that relay handles "envelopes" and the services also stuff messages into envelopes to send each other messages. But I can't think of any better naming for now.

@tobias-wilfert tobias-wilfert marked this pull request as ready for review August 16, 2022 14:29
@tobias-wilfert tobias-wilfert requested a review from a team August 16, 2022 14:29
relay-server/src/utils/actix.rs Outdated Show resolved Hide resolved
.enable_all()
.build()
.unwrap();
let runtime = utils::construct_runtime();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this now creates a runtime which can send messages to actix, but it doesn't really need that. I'd be fine with leaving this as calling the tokio builder directly, I don't mind a bit of being explicit as there's no tricky stuff going on like setting up the actix system that needs to be abstracted and documented. (I appreciate @jan-auer may have desired to hide the tokio builder, I'll politely disagree 😉 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few reasons for keeping this abstraction:

  • There will be plenty of runtimes constructed for all the services once they are migrated. This is a single line and makes it easier to read, writing a 5+ line block every time will just clutter our constructor functions.
  • It gives us a single place to hook central concerns or enforce patterns. For example, in a follow-up I will require to set explicit names for every runtime, so this can simply become a required parameter of that function.
  • Initializing the actix system for every runtime is actually desired here, since that removes any risk of accidentally forgetting it. Also, this means there can be a single TODO in the code base to remove it after the migration is done.

We will need to pass a flag or count to control whether this should be a single- or multi-threaded runtime, however.

relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
@flub
Copy link
Contributor

flub commented Aug 16, 2022

Just a few small nits, but overall looks great.

relay-server/src/actors/healthcheck.rs Outdated Show resolved Hide resolved
relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-system/src/lib.rs Outdated Show resolved Hide resolved
@flub
Copy link
Contributor

flub commented Aug 17, 2022

If we're renaming Envelope to Messages let's make sure to do that also for Service::Envelope and ServiceMessage::into_envelope. But at that point ServiceMessage is also a naming oddity and should probably just become Message?

Comment on lines +19 to +22
mod service;

pub use self::controller::*;
pub use self::service::*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you say to make pub mod service and remove the service::* use? This is how you use it in #1421 (and the missing pub is why that's currently unhappy) and it kind of makes sense (i'm generally not a fan of * imports)

Copy link
Member Author

@tobias-wilfert tobias-wilfert Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was related to a comment by Jan:

In this case, I'd rather make this a private module and do a blanket export like for controller. This module will contain the most used types of this crate, so it's worth having them in the root namespace.

So I changed it here but forgot to change it in #1421 but that should be fixed now.

relay-server/src/actors/store.rs Outdated Show resolved Hide resolved
relay-server/src/utils/actix.rs Outdated Show resolved Hide resolved
@tobias-wilfert tobias-wilfert changed the title ref(actix): Update Store Actor ref(actix): Update Store Actor [INGEST-1565] Aug 18, 2022
@tobias-wilfert tobias-wilfert merged commit ee42532 into master Aug 18, 2022
@tobias-wilfert tobias-wilfert deleted the tobias-wilfert/future-proofing-relay branch August 18, 2022 10:53
@tobias-wilfert tobias-wilfert mentioned this pull request Aug 18, 2022
30 tasks
@HazAT HazAT added this to the Upgrade Tokio in Relay milestone Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants