fix(replication): replace custom AbortToken with tokio::CancellationToken#1131
fix(replication): replace custom AbortToken with tokio::CancellationToken#1131meskill wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
|
||
| // Create a child cancel token with the guard to cancel the handles below | ||
| // in case any of it fails without affecting the parent task. | ||
| // If every handle succeed the guard token will just cancel already finished work |
There was a problem hiding this comment.
| // If every handle succeed the guard token will just cancel already finished work | |
| // If every handle succeeds the guard token will just cancel already finished work |
| // Create a child cancel token with the guard to cancel the handles below | ||
| // in case any of it fails without affecting the parent task. | ||
| // If every handle succeed the guard token will just cancel already finished work | ||
| let cancel = cancel.child_token(); |
There was a problem hiding this comment.
I think we should require the caller to call .child_token() if they don't want to be cancelled.
| let cancel = cancel.child_token(); |
There was a problem hiding this comment.
I think it's better to leave it here, in the ParallelSyncManager. In the run method we start multiple ParallelSync instances to copy tables in parallel. So, basically ParallelSyncManager manager the parallel work here and the cancel token is here to help manage it: we create a child_token with guard to not affect parent or sibling work, but still be affected if the parent is cancelled. The guard is needed to cancel all the other running syncs in case any of them fails: in that case we cancel this execution subtree and propagate the error to parent where the parent will react on error and finish with the error eventually and not cancellation.
| // in case any of it fails without affecting the parent task. | ||
| // If every handle succeed the guard token will just cancel already finished work | ||
| let cancel = cancel.child_token(); | ||
| let _guard = cancel.clone().drop_guard(); |
There was a problem hiding this comment.
| let _guard = cancel.clone().drop_guard(); | |
| let _guard = cancel.drop_guard_ref(); |
| let cancel = cancel.clone(); | ||
| syncs.push(async move { | ||
| let manager = ParallelSyncManager::new(tables, replicas, dest)?; | ||
| let tables = manager.run().await?; | ||
| let tables = manager.run(cancel).await?; |
There was a problem hiding this comment.
| let cancel = cancel.clone(); | |
| syncs.push(async move { | |
| let manager = ParallelSyncManager::new(tables, replicas, dest)?; | |
| let tables = manager.run().await?; | |
| let tables = manager.run(cancel).await?; | |
| syncs.push(async move { | |
| let manager = ParallelSyncManager::new(tables, replicas, dest)?; | |
| let tables = manager.run(cancel.child_token()).await?; |
|
|
||
| let dest = dest.clone(); | ||
| set.spawn(async move { | ||
| let cancel = cancel.clone(); |
There was a problem hiding this comment.
This is the first place we actually do anything with a cancellation token other than pass it through. Should we remove everything up the chain from here and the CancellationToken::new()s, and create it at this point?
There was a problem hiding this comment.
The idea here, that the initial CancellationToken comes from the
pgdog/pgdog/src/api/async_task.rs
Lines 358 to 361 in 02c7a32
so the whole task becomes manageable from there. We create the token in the root and pass it down to any child task. That's why it passed here and just cloned or child_token is created.
Replace custom AbortSignal with CancellationToken to make the cancellation explicit and not relied on futures drop (that holds rx part of channel). Pass cancellation token for graceful cancellation. Early cancellation on errors