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.

Sign up for GitHub

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

Jump to bottom

Module hotswapping #1147

Merged
merged 1 commit into from
May 22, 2024
Merged

Module hotswapping #1147

merged 1 commit into from
May 22, 2024

Conversation

coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Apr 23, 2024 •
edited

Description of Changes

This needs some testing. I feel like there's something obvious I'm missing here but this like, should work. At some point, I should write some smoketests about how clients behave when the module disconnects.

Expected complexity level and risk

3 (touches the websocket message actor - though I'm confident about that part. moreso just that I'm not sure that I preserve all the module lifecycle stuff.)

Testing

  • quickstart-chat hotswapping
  • Bots testing with hotswapping a module in.

@coolreader18 coolreader18 added the test-with-bots Recommend to test under higher CCU label Apr 23, 2024
coolreader18
coolreader18 commented Apr 23, 2024
View reviewed changes
crates/core/src/host/host_controller.rs Outdated
@@ -240,7 +249,6 @@ impl HostController {
energy_monitor: self.energy_monitor.clone(),
};
let module_host = spawn_rayon(move || Self::make_module_host(mhc.host_type, mcc)).await?;
module_host.start();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not necessary anymore - we always start the module immediately now, so there's no point in having the start functionality anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

So why are we starting it immediately now?

@coolreader18 coolreader18 linked an issue Apr 24, 2024 that may be closed by this pull request
Hotswap modules without disconnecting clients #1148
Closed
@jdetter
Copy link
Collaborator

jdetter commented Apr 25, 2024

Bot test resulted in the client not receiving any more transactions after the database update was deployed. The client doesn't get disconnected but I can't call any reducers and it appears that I no longer receive any updates

@bfops bfops added the release-any To be landed in any release window label Apr 29, 2024
@coolreader18 coolreader18 force-pushed the noa/module-hotswap branch 2 times, most recently from 01f9788 to 3ba5470 Compare April 30, 2024 20:29
@coolreader18
Copy link
Collaborator Author

coolreader18 commented Apr 30, 2024 •
edited

Currently with this branch, calling update_database may invoke the __update__ reducer and thus send an event to subscribers before the new module is fully inserted into the database. I'm not sure of a good way to prevent this.

@coolreader18 coolreader18 requested a review from kim May 1, 2024 17:48
@kim
Copy link
Contributor

kim commented May 2, 2024

Currently with this branch, calling update_database may invoke the __update__ reducer and thus send an event to subscribers before the new module is fully inserted into the database. I'm not sure of a good way to prevent this.

I believe this is fine, or at least after #1186 lands. __update__ runs in a transaction, so if it succeeds it is technically correct to send the event.

kim
kim reviewed May 2, 2024
View reviewed changes
crates/core/src/database_instance_context.rs
@@ -15,6 +16,7 @@ pub struct DatabaseInstanceContext {
pub database: Database,
pub database_instance_id: u64,
pub logger: Arc<DatabaseLogger>,
pub subscriptions: ModuleSubscriptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that I missed ModuleSubscriptions in #1186. If the database panics during eval, it must be dropped and removed from the host controller.

Or maybe that doesn't matter, because if a read-only transaction panics we're in a hosed state anyway. Not sure.

crates/core/src/host/host_controller.rs Outdated
@@ -240,7 +249,6 @@ impl HostController {
energy_monitor: self.energy_monitor.clone(),
};
let module_host = spawn_rayon(move || Self::make_module_host(mhc.host_type, mcc)).await?;
module_host.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

So why are we starting it immediately now?

kim
kim approved these changes May 2, 2024
View reviewed changes
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

Tested with quickstart-chat.
Code LGTM.

@coolreader18
Copy link
Collaborator Author

coolreader18 commented May 2, 2024 •
edited

So why are we starting it immediately now?

I think that was your change @kim - .start() used to be called after some other setup was done, but if we're always calling it immediately after constructing the module, we may as well just not have the mechanism for it.

@kim
Copy link
Contributor

kim commented May 4, 2024

I think that was your change @kim - .start() used to be called after some other setup was done

Hm I don’t know that I did that — but we do need to start it, because we’re almost certainly going to call a reducer, no?

@coolreader18
Copy link
Collaborator Author

It used to be .start()ed after calling init on it, but not anymore.

@kim kim mentioned this pull request May 15, 2024
Merged
2 tasks
@coolreader18 coolreader18 mentioned this pull request May 17, 2024
Closed
@coolreader18 coolreader18 force-pushed the noa/module-hotswap branch 3 times, most recently from cd203d2 to 6bf74b7 Compare May 20, 2024 21:42
@cloutiertyler cloutiertyler added this pull request to the merge queue May 22, 2024
Merged via the queue into master with commit 6b1a3d3 May 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kim kim kim approved these changes

@jdetter jdetter Awaiting requested review from jdetter

Assignees
No one assigned
Labels
release-any To be landed in any release window test-with-bots Recommend to test under higher CCU
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hotswap modules without disconnecting clients
5 participants
@coolreader18 @jdetter @kim @bfops @cloutiertyler