feat: conflict resolution scaffolding#687
Conversation
dav1do
force-pushed
the
feat/conflict-resolution
branch
3 times, most recently
from
054952e to
5cbbbb0
Compare
| reason: String, | ||
| }, | ||
| /// 'Soft error' -> should not kill recon conversation but should not be persisted | ||
| /// A time event could not be validated because no RPC provider was available |
There was a problem hiding this comment.
| /// A time event could not be validated because no RPC provider was available | |
| /// E.g. a time event could not be validated because no RPC provider was available |
There was a problem hiding this comment.
I'm considering removing this error (and actually have for now but let's discuss).
Do we want to let the Recon conversation continue if a Time Event couldn't be validated?
- If it's a transient issue (like RPC rate limiting), then it'll eventually go away.
- If it's a Time Event for which the node should have a RPC endpoint configured but doesn't, then we want the failure to be loud so that the operator fixes their configuration.
- If it's a bad Time Event, then the Recon conversation should be terminated.
WDYT?
There was a problem hiding this comment.
I think validation needs to raise different types of errors for those different cases. Transient errors should let the conversation continue, permanent errors should kill the conversation
There was a problem hiding this comment.
Ok, I'm adding a TODO here to look at in the next PR.
| type Result = anyhow::Result<Option<StreamState>>; | ||
| } | ||
|
|
||
| /// State of a single stream |
There was a problem hiding this comment.
I'm surprised we don't already have a type for this? Don't we already have a stream_states table that must have a type for this?
There was a problem hiding this comment.
The pipeline naming is generally a bit confusing, and also, there are two instances of this struct, one in the Aggregator (aggregator.rs) and another in the Resolver (resolver.rs).
In the Aggregator (which applies commit patches), it's used with the event_states table, while in the Resolver, it's used with the stream_states table.
I could rename it to EventState in the Aggregator. It isn't really the stream state at that stage of the pipeline because the true stream state can only be determined after conflict resolution in the Resolver.
WDYT?
There was a problem hiding this comment.
definitely think they should have different names if they have different meanings. Maybe this one is ResolvedStreamState or something like that?
smrz2001
force-pushed
the
feat/conflict-resolution
branch
from
5cbbbb0 to
cf69d03
Compare
smrz2001
changed the title
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
it will require a db connection so this avoids circular deps, plus it doesn't need to be in the validation crate
… conclusion events
Co-authored-by: Spencer T Brody <spencer+github@3box.io>
Co-authored-by: Spencer T Brody <spencer+github@3box.io>
smrz2001
force-pushed
the
feat/conflict-resolution
branch
from
79d62c2 to
d65b9e2
Compare
4 participants
The meat of this PR can be found in its comments. There are several TODO comments. At a high level there are two halfs to completing conflict resolution:
The conflict resolver will need the before time data as well as the previous fields in addition to the data already present on the event_states table. Getting previous field will be straightforward as the conclusion_events already have that data but its not preserved into the event_states table.
Annotation the conclusion events with their before times has two challenges. First while we validate events we promptly forget the block time stamp of the event time proof. We need to preserve that information. Second its not clear how to propagate the before conclusion from time events to data events if that is even needed. I would recommend starting on part two to understand exactly what data the resolver UDF needs and then work backwards to the concluder to determine how to get that information.