Timer Table Implementation #1449

Shubham8287 · 2024-06-19T18:52:21Z

Description of Changes

Host side implementation of scheduled tables as per proposal - https://github.com/clockworklabs/SpacetimeDBPrivate/blob/master/proposals/0005-timers.md

API and ABI breaking changes

yes

Expected complexity level and risk

3

Testing

Tested via running through example apps.

kazimuth · 2024-06-20T18:22:32Z

crates/sats/src/db/def.rs

@@ -485,6 +485,7 @@ pub struct TableSchema {
    pub sequences: Vec<SequenceSchema>,
    pub table_type: StTableType,
    pub table_access: StAccess,
+    pub scheduled: Option<Box<str>>,


In the PR #1448 I'm rewriting Def. My plan was to add:

struct ScheduleDef { reducer_name: Box<str>, at_column: ColId }

And then having an Option<ScheduleDef> on TableDef.
If you don't mind restructuring this that way it would make it easier for me down the line.

How do you translate this in TableSchema?

I guess just assert that at_column is named "at" and convert the reducer name.

This can be changed later once TableSchema is updated.

Centril · 2024-06-20T16:19:39Z

crates/core/src/db/datastore/locking_tx_datastore/state_view.rs

+            .map(|row| -> Result<_> {
+                let row = StScheduledRow::try_from(row)?;
+                Ok(row.reducer_name)
+            })
+            .transpose()?;


Suggested change

.map(|row| -> Result<_> {

let row = StScheduledRow::try_from(row)?;

Ok(row.reducer_name)

})

.transpose()?;

.and_then(|row| StScheduledRow::try_from(row).map(|r| r.reducer_name))?

ummm, scheduled is supposed to be an Option enum, provided suggestion does not looks to evaluates to that.

Given that @Centril did not provide more context I would mark this as resolved.

crates/core/src/db/datastore/system_tables.rs

crates/core/src/db/relational_db.rs

crates/lib/src/timestamp.rs

crates/lib/src/scheduler.rs

crates/core/src/host/scheduler.rs

crates/core/src/host/wasm_common/module_host_actor.rs

crates/core/src/host/scheduler.rs

crates/core/src/host/wasm_common.rs

Centril · 2024-06-27T09:47:28Z

crates/core/src/host/scheduler.rs

+// can this be shared with bindings?
+const SCHEDULED_AT_FIELD: &str = "scheduled_at";
+const SCHEDULED_ID_FIELD: &str = "scheduled_id";


Yes, you can move them to spacetimedb_primitives, e.g., scheduling.rs.

Centril · 2024-06-27T09:57:34Z

crates/core/src/host/scheduler.rs


 #[derive(Copy, Clone, Eq, PartialEq, Hash)]
-pub struct ScheduledReducerId(pub u64);
+pub struct ScheduledReducerId {
+    /// Scheduled Table id, this should have a entry in `ST_SCHEDULED`


@Shubham8287 This hasn't been addressed.

kim

Looks good overall.
I'll want to do the manual testing before approving.

crates/client-api/src/routes/database.rs

crates/core/src/db/relational_db.rs

crates/core/src/host/instance_env.rs

crates/core/src/host/module_host.rs

kim · 2024-06-27T13:02:15Z

crates/core/src/host/scheduler.rs

+                                            db.delete(tx, id.table_id, [schedule_row_ptr]);
+                                            Ok::<(), anyhow::Error>(())
+                                        })
+                                        .map_err(|e| log::error!("deleting scheduled reducer failed: {e:#}"));


Strangely, the types are lying -- this transaction cannot actually fail.

But if it could, we might run the scheduler again in the future, is that correct?

I think it's important to note both in the proposal and user-facing documentation that scheduled reducers have "at least once" semantics.

But does that also means that there is a bug in our logic?

I think it means that, like in many instances in the codebase, we formulate types not to express what is the case, but what might be the case in the future.

In this case: transactions may become fallible under MVCC, if and when that is supported, but they are, in fact, not.

If we resist debating the code structure, the possibility of this particular transaction being fallible means that we may run the scheduled reducer more than once.

I think that’s fine, but we need to be diligent in documenting the behavior.

crates/core/src/host/scheduler.rs

crates/lib/src/scheduler.rs

crates/core/src/host/scheduler.rs

crates/core/src/host/module_host.rs

crates/core/src/host/scheduler.rs

kim · 2024-07-02T08:43:29Z

crates/core/src/host/scheduler.rs

+                Ok(schedule_row) => schedule_row,
+                // if the row is not found, it means the schedule is cancelled by the user
+                Err(_) => {
+                    log::info!(


Should we downgrade this to debug?

I created a reducer with ScheduleAt::Interval(duration!(100ms)), and then delete the the table in the first call. That yields 3x, supposedly because it was put on the queue this many times already.

It's not immediately clear what the message is trying to tell me.

Did you put it inside init?

So it was extra noisy for you due to bug specific for this particular case - https://discord.com/channels/931210784011321394/1011381307965722774/1257812281564073996, but I think message can still be downgraded to Debug, purpose for it is for debugging only.

crates/bindings-macro/src/lib.rs

Centril

Looks like the test suite is failing for trivial reasons; with that fixed, this look good!

Update test snapshot Add support for the new timer tables Remove old scheduler ABI & API Add implicit conversions for ScheduleAt

Shubham8287 · 2024-07-16T09:38:30Z

new commits after last update are

Few times rebasing with master.
merging [DO NOT MERGE] Timer tables implementation for C# #1459, which was required for smoketests to pass.
Simplify Timestamp usage which was conflicting with protobufectomy.
All of them seems trivial or not to be concerned with PR main logic, hence didn't seek new approval.

Shubham8287 self-assigned this Jun 20, 2024

Shubham8287 force-pushed the shub/scheduler-actor branch 2 times, most recently from c0db594 to b13cf7c Compare June 20, 2024 16:45

kazimuth reviewed Jun 20, 2024

View reviewed changes

Centril reviewed Jun 20, 2024

View reviewed changes

kim reviewed Jun 21, 2024

View reviewed changes

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

bfops added the release-any To be landed in any release window label Jun 24, 2024

Shubham8287 force-pushed the shub/scheduler-actor branch from 77d405b to e951eb7 Compare June 26, 2024 09:05

Shubham8287 added abi-break A PR that makes an ABI breaking change release-0.11 and removed release-any To be landed in any release window labels Jun 26, 2024

Shubham8287 commented Jun 26, 2024

View reviewed changes

crates/core/src/host/wasm_common.rs Outdated Show resolved Hide resolved

Shubham8287 force-pushed the shub/scheduler-actor branch 5 times, most recently from 72bab7c to 15bf757 Compare June 26, 2024 14:43

Shubham8287 requested review from Centril and kim June 26, 2024 14:44

Shubham8287 force-pushed the shub/scheduler-actor branch from 15bf757 to 85a0aed Compare June 26, 2024 14:55

Shubham8287 changed the title ~~Shub/scheduler actor~~ scheduled tables Jun 26, 2024

Shubham8287 force-pushed the shub/scheduler-actor branch 2 times, most recently from 87f7dc6 to 1b3bbe0 Compare June 26, 2024 17:15

Centril reviewed Jun 27, 2024

View reviewed changes

kim reviewed Jun 27, 2024

View reviewed changes

Shubham8287 force-pushed the shub/scheduler-actor branch from 92fff9f to 9f5c008 Compare June 27, 2024 13:41

Centril reviewed Jun 27, 2024

View reviewed changes

crates/lib/src/scheduler.rs Outdated Show resolved Hide resolved

crates/lib/src/scheduler.rs Outdated Show resolved Hide resolved

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

Centril reviewed Jun 27, 2024

View reviewed changes

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

Shubham8287 force-pushed the shub/scheduler-actor branch from b1c2b45 to be97bda Compare June 28, 2024 14:31

Centril reviewed Jun 28, 2024

View reviewed changes

crates/core/src/host/module_host.rs Outdated Show resolved Hide resolved

crates/core/src/host/scheduler.rs Outdated Show resolved Hide resolved

kim reviewed Jul 2, 2024

View reviewed changes

Shubham8287 force-pushed the shub/scheduler-actor branch 4 times, most recently from 20bc515 to 47dedc6 Compare July 3, 2024 08:49

Centril approved these changes Jul 3, 2024

View reviewed changes

joshua-spacetime changed the title ~~scheduled tables~~ Timer Table Implementation Jul 8, 2024

Add ScheduleAt and time helpers

3bd5a39

Update test snapshot Add support for the new timer tables Remove old scheduler ABI & API Add implicit conversions for ScheduleAt

Shubham8287 force-pushed the shub/scheduler-actor branch 3 times, most recently from 6c2b51f to c9b9699 Compare July 15, 2024 19:18

Shubham8287 added 2 commits July 16, 2024 01:51

Scheduled tables

0e0b73e

Merge branch 'shub/csharp-scheduler-table' into shub/scheduler-actor

7516f72

Shubham8287 force-pushed the shub/scheduler-actor branch from c9b9699 to 7516f72 Compare July 16, 2024 08:03

Shubham8287 added this pull request to the merge queue Jul 16, 2024

Merged via the queue into master with commit 276387d Jul 16, 2024
14 checks passed

Shubham8287 mentioned this pull request Jul 16, 2024

[DO NOT MERGE] Timer tables implementation for C# #1459

Closed

2 tasks

bfops mentioned this pull request Jul 16, 2024

C# bindings for scheduled tables #1327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timer Table Implementation #1449

Timer Table Implementation #1449

Shubham8287 commented Jun 19, 2024 •

edited

Loading

kazimuth Jun 20, 2024

Shubham8287 Jun 26, 2024

kazimuth Jul 12, 2024 •

edited

Loading

Centril Jun 20, 2024

Shubham8287 Jun 26, 2024

cloutiertyler Jul 16, 2024

Centril Jun 27, 2024 •

edited

Loading

Centril Jun 27, 2024

kim left a comment

kim Jun 27, 2024

Shubham8287 Jun 28, 2024

kim Jun 28, 2024

kim Jul 2, 2024

Shubham8287 Jul 2, 2024

kim Jul 3, 2024

Shubham8287 Jul 3, 2024

Centril left a comment

Shubham8287 commented Jul 16, 2024

Timer Table Implementation #1449

Timer Table Implementation #1449

Conversation

Shubham8287 commented Jun 19, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazimuth Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril left a comment

Choose a reason for hiding this comment

Shubham8287 commented Jul 16, 2024

Shubham8287 commented Jun 19, 2024 •

edited

Loading

kazimuth Jul 12, 2024 •

edited

Loading

Centril Jun 27, 2024 •

edited

Loading