Table change check #80
Conversation
twuebi
force-pushed
the
tp/change-check-wip
branch
from
b586b67
to
ab6162f
Compare
twuebi
force-pushed
the
tp/change-check-wip
branch
from
ab6162f
to
12311bc
Compare
twuebi
force-pushed
the
tp/change-check-wip
branch
from
9074bfc
to
a25f479
Compare
| .v1_state | ||
| .contract_verifiers | ||
| .check(&update, *table_uuid, &response.previous_table_metadata) | ||
| .await? |
There was a problem hiding this comment.
There might be quite a few updates here - I would prefer concurrent execution and try_join
There was a problem hiding this comment.
It doesn't quite fit here, but I am also a bit hesitant about awaiting the emit_change_event block.
We currently implement a "send and forget" approach. But right now, if the event queue is offline, we are blocking the client response until the event queue gets a timeout.
What's your feeling about emitting events non-blocking?
There was a problem hiding this comment.
Do we expect this endpoint to be called a lot? How many changes do we expect per call?
We could separate it out via a channel and have some background task shoveling it into nats.
There was a problem hiding this comment.
There might be quite a few updates here - I would prefer concurrent execution and try_join
made it futures + try_join_all
There was a problem hiding this comment.
Do we expect this endpoint to be called a lot? How many changes do we expect per call?
We could separate it out via a channel and have some background task shoveling it into nats.
| @@ -819,6 +842,19 @@ impl<C: Catalog, A: AuthZHandler, S: SecretStore> | |||
| )); | |||
| } | |||
|
|
|||
| for ((update, (_, table_uuid)), response) in updates | |||
There was a problem hiding this comment.
I think we should push the whole block up to right after C::commit_table_transaction.
There is no need to load storage secret if the contract check fails.
| @@ -929,3 +965,65 @@ fn maybe_body_to_json(request: impl Serialize) -> serde_json::Value { | |||
| serde_json::Value::Null | |||
| } | |||
| } | |||
|
|
|||
| // TableUpdate is not clone but all containing fields are so we use this function to clone.. | |||
| fn copy_updates(updates: &[TableUpdate]) -> Vec<TableUpdate> { | |||
There was a problem hiding this comment.
Should we maybe rename to clone_updates?
| async fn check( | ||
| &self, | ||
| table_updates: &[TableUpdate], | ||
| table_ident_uuid: TableIdentUuid, |
There was a problem hiding this comment.
Now that we send the current_metadata, we might want to skip the table_ident_uuid.
The table-uuid is already part of TableMetadata and might change during updates.
If we specify it here additionally, we should rename it to current_table_ident_uuid to make sure it is the UUID before updates.
I think removing it is fine though.
There was a problem hiding this comment.
thanks, missed it in table-metadata, removed!
| Ok(ContractVerificationOutcome::Clear {}) => {} | ||
| Ok(block_result @ ContractVerificationOutcome::Violation { error_model: _ }) => { | ||
| tracing::info!( | ||
| "Checker {} blocked change on table '{}'", |
There was a problem hiding this comment.
Lets reflect the trait in the log - maybe ContractValidator {} blocked change on table ..
Also for other logs here.
#58