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

Remove module update functionality in favor of future migration/publish argument implementation #1370

Closed
cloutiertyler opened this issue Jun 11, 2024 · 7 comments · Fixed by #1557
Assignees

Comments

@cloutiertyler
Copy link
Contributor

Callback when publishing without -c

Problem

The BitCraft team wants some custom logic to execute when they publish a module without using -c. One thing Alexander wants to be able to do is change the duration of a scheduled reducer (because the durations are part of the static data, which they can update from the client).

Proposed Solution #1

We could add a callback function that is analogous to the init(..) function, except its executed when publishing without -c. Basically in this new update(..) function he could lookup any scheduled reducers, cancel them all, and then reschedule them with the new updated value.

Perhaps we decorate like this:

#[spacetimedb(init)]
pub fn init() {
  // Do stuff in here that should only be done when starting from scratch
}

#[spacetimedb(update)]
pub fn update() {
  // Do stuff in here that should be done when starting from scratch *or* when a module is updated
}

This solution is in the same vein as a migration handler. As specified here this function would not have any ability to migrate any data, but it would give you a handle to be able to do operations on your module

Some questions to answer before we move forward on this:

  • As part of the module lifecycle, maybe init is called when you publish with -c and module_updated(..) is also called when you publish with -c, but when you publish without -c only module_updated(..) is called.

Proposed Solution #2

(pasted from comments section)

For now we could just tell the BitCraft team to refactor their code. Any code that manages repeating reducers should be in a new reducer called manage_reducers(..) . This reducer would be called from init, but it would also be manually invoked after publishing without -c.

This ticket is about number 1 here:*

  1. You haven’t changed the schema and you’re intending to update the module and you want to run something when that happens
  2. You have changed the schema, but you haven’t written a migration function. If we continue with this you will be in a borked state so we should throw an error
  3. You want to do a migration, you’ve done a schema change and you’ve written a migration function.
@cloutiertyler
Copy link
Contributor Author

I believe this is possibly already implemented, but we should use this ticket to make sure we're happy with the current design.

@kim
Copy link
Contributor

kim commented Jun 18, 2024

Yes we have that: init is called on the first publish, and update is called on every subsequent update (aka publish-without-dash-c). Both are transactional, i.e. if the reducers return an error, nothing is committed to the database and either no database is running (on init), or the old database continues running (on update).

@cloutiertyler
Copy link
Contributor Author

cloutiertyler commented Jul 15, 2024

One thing I would like to review for the current design is that if your update function is not idempotent, then publishing a module twice without changing the code will apply the effects twice which might be surprising behavior. Not sure if we care about that though.

Perhaps we just remove this capability and leave it's implementation to be run as a migration with the migration UX

@jdetter
Copy link
Collaborator

jdetter commented Jul 15, 2024

Possible solution: when you publish a database update you can optionally specify a function to call when the update is applied. e.g.:

#[spacetimedb(init)]
pub fn init(...) {}

// This macro makes it possible to call this reducer during update
#[spacetimedb(update)]
pub fn do_module_update(...) {}

Then to publish:

spacetime publish -s  --update='do_module_update' my_module_name

@kim
Copy link
Contributor

kim commented Jul 16, 2024

I like the idea of doing this via migrations -- as timer tables are just tables, even spawning a repeated task would fit into the model.

Perhaps we just remove this capability

Maybe replace with migrations.

@cloutiertyler
Copy link
Contributor Author

I think we can all agree we want to remove the update function as is. That's technically the only ABI breaking thing. I don't think BitCraft relies on this so we should just pull it out for this release at least.

@cloutiertyler cloutiertyler assigned kim and unassigned jdetter Jul 29, 2024
@cloutiertyler
Copy link
Contributor Author

I'm going to rename this to "Remove update functionality in favor of future migration/publish argument implementation"

@cloutiertyler cloutiertyler changed the title Callback when publishing without -c Remove module update functionality in favor of future migration/publish argument implementation Jul 29, 2024
@kim kim linked a pull request Jul 30, 2024 that will close this issue
@kim kim closed this as completed in #1557 Jul 31, 2024
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 a pull request may close this issue.

3 participants