Conversation
Confirmed reads applies only to subscription clients, calls to the the HTTP API publish endpoint return a success response before the operation is confirmed. While we await scheduling of a new database, updates require to wait for the update transaction to be confirmed. To allow this, the `TransactionOffset` channel and the database's `DurableOffset` need to be returned all the way up to the request handler. Note that waiting for confirmation is almost always the right choice, so can't be opted out of at the time of submission of this patch. Callers may, however, extend the timeout after which waiting for confirmation is cancelled.
| UpdateDatabaseResult::ErrorExecutingMigration(anyhow::anyhow!(msg)) | ||
| } else { | ||
| UpdateDatabaseResult::UpdatePerformed | ||
| let tx_offset = succeed(self.info.clone(), out.energy_used.into(), out.total_duration, tx); |
There was a problem hiding this comment.
Moving the code around for borrowchk reasons, I see that energy_quanta_used and host_execution_duration only have non-zero values in this branch (ie. EvaluateSubscribedViews).
Is this intentional and correct?
There was a problem hiding this comment.
Hm. It seems like there should be some energy cost to performing an update, but clearly that's separate work from this PR. I've created #4847 to track this.
| fn default_update_confirmation_timeout() -> Duration { | ||
| Duration::from_secs(5) |
There was a problem hiding this comment.
Duration::from_secs is const, so this could be replaced with const DEFAULT_UPDATE_CONFIRMATION_TIMEOUT: Duration = Duration::from_secs(5);.
| /// Duration to wait for a database update to become confirmed (i.e. durable). | ||
| /// | ||
| /// If not given, defaults to [default_update_confirmation_timeout]. | ||
| /// The parameter has no effect when creating a new database. | ||
| update_confirmation_timeout: Option<Duration>, |
There was a problem hiding this comment.
Do we want to impose an upper bound on this?
| request_id: None, | ||
| timer: None, | ||
| }; | ||
| //TODO: Return back event in `UpdateDatabaseResult`? |
| UpdateDatabaseResult::ErrorExecutingMigration(anyhow::anyhow!(msg)) | ||
| } else { | ||
| UpdateDatabaseResult::UpdatePerformed | ||
| let tx_offset = succeed(self.info.clone(), out.energy_used.into(), out.total_duration, tx); |
There was a problem hiding this comment.
Hm. It seems like there should be some energy cost to performing an update, but clearly that's separate work from this PR. I've created #4847 to track this.
|
Oh, something I forgot to include in my review: I agree that this is a reasonable behavior, and I don't see the need to make it more configurable than this. |
2 participants
Confirmed reads applies only to subscription clients, calls to the the
HTTP API publish endpoint return a success response before the operation
is confirmed.
While we await scheduling of a new database, updates require to wait for
the update transaction to be confirmed. To allow this, the
TransactionOffsetchannel and the database'sDurableOffsetneed tobe returned all the way up to the request handler.
Note that waiting for confirmation is almost always the right choice, so
can't be opted out of at the time of submission of this patch. Callers
may, however, extend the timeout after which waiting for confirmation is
cancelled.