Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

chainHead: Add support for storage pagination and cancellation #14755

Merged
merged 19 commits into from
Aug 24, 2023
Merged

chainHead: Add support for storage pagination and cancellation #14755

merged 19 commits into from
Aug 24, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 11, 2023

This PR adds configurable pagination for the storage query types: DescendantHashes and DescendantValues with the possibility to cancel the operation after the WaitingForContinue event is generated.

  • implement chainHead_unstable_continue method
  • implement chainHead_unstable_stopOperation method
  • add configurable pagination limit after which the WaitingForContinue event is generated
  • iterable storage queries are placed in a queue for providing fairness across multiple queries of the same operation
  • limiter object from chainHead: Limit ongoing operations #14699 is refactored inside an Operations object for better ergonomics
  • registering an operation shares a lock-free state between the backend and frontend for efficient signaling of continue or stop
    • RegisteredOperation object is returned to the backend (chainHead event generation) via the BlockGuard
    • OperationState object is returned to the frontend and has direct user access via chainHead_unstable_continue and chainHead_unstable_stopOperation
  • RegisteredOperation in RAII manner (on drop) releases the capacity permit for the operation and unregisters the operation from the subscription state
  • integration-tests for the continue and stop operations (via check_continue_operation and stop_storage_operation)

Builds on top of: #14699
Closes: #14549
Closes: #14639

// @paritytech/subxt-team

@lexnv lexnv added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T2-API This PR/Issue is related to APIs. labels Aug 11, 2023
@lexnv lexnv requested review from jsdw and skunert August 11, 2023 11:33
@lexnv lexnv self-assigned this Aug 11, 2023
@lexnv lexnv changed the title chainHead: Add support storage pagination and cancellation chainHead: Add support for storage pagination and cancellation Aug 11, 2023
Base automatically changed from lexnv/chainhead_limits to master August 15, 2023 12:17
@lexnv
chainHead/api: Add chain_head_unstable_continue method
a97fd42 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscriptions: Register operations for pagination
fe3404c 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscriptions: Merge limits with registered operation
72b26c3 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscriptions: Expose the operation state
e28d982 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chain_head/storage: Generate WaitingForContinue event
1d717df 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead: Use the continue operation
3bbc9ba 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/tests: Adjust testing to the new storage interface
5afe9c0 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/config: Make pagination limit configurable
c8e8ed5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/tests: Adjust chainHeadConfig
60492c7 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/tests: Check pagination and continue method
6929ec5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/api: Add chainHead_unstable_stopOperation method
2791c90 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscription: Add shared atomic state for efficient alloc
8bda477 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead: Implement operation stop
382ce98 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/tests: Check that storage ops can be cancelled
3a5c12a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/storage: Change docs for query_storage_iter_pagination
6d825a6 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscriptions: Fix merge conflicts
cab6202 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
niklasad1
niklasad1 reviewed Aug 22, 2023
View reviewed changes
client/rpc-spec-v2/Cargo.toml Outdated
@@ -36,6 +36,7 @@ tokio = { version = "1.22.0", features = ["sync"] }
array-bytes = "6.1"
log = "0.4.17"
futures-util = { version = "0.3.19", default-features = false }
async-channel = "1.8.0"
Copy link
Member

@niklasad1 niklasad1 Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why async-channel is used here?

You already got the futures-channel and tokio sync in the dependency tree so I don't follow why this dependency is introduced as the both tokio and futures-channel provides an API for the stuff you implemented on top of the channel

EDIT: tokio::sync::mpsc creates a buffer with 1 and futures_channel a buffer with 1 + num senders, so I guess you want exactly one here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! I can't actually remember if I picked the async-channel here since it doesn't require the receiver part to be mutable 🤔

I'll change a bit the API since it's worth not adding extra dependencies, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, didn't notice that :D

niklasad1
niklasad1 reviewed Aug 23, 2023
View reviewed changes
client/rpc-spec-v2/src/chain_head/subscription/inner.rs
.requested_continue
.store(true, std::sync::atomic::Ordering::Release);

let _ = self.recv_continue.recv().await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the channel is closed i.e, returns TrySendError::Closed or can't that happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sender part of the channel should be around until this RegisteredOperation is dropped, since it object is the only one that can remove the operation from tracking (the operations: Arc<Mutex<HashMap<String, OperationState>>> field), will add a comment :D

@lexnv
chainHead: Replace async-channel with tokio::sync
141ca3f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
chainHead/subscription: Add comment about the sender/recv continue
7ccef40 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv mentioned this pull request Aug 23, 2023
Open
skunert
skunert approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice!

niklasad1
niklasad1 approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@lexnv lexnv merged commit 92633bb into master Aug 24, 2023
49 checks passed
@lexnv lexnv deleted the lexnv/chainhead_pag branch August 24, 2023 11:32
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skunert skunert skunert approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

@jsdw jsdw Awaiting requested review from jsdw

Assignees

@lexnv lexnv

Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T2-API This PR/Issue is related to APIs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RPC-Spec-V2] Cancel ongoing operations [RPC-Spec-V2] Storage: Add support for pagination
3 participants
@lexnv @skunert @niklasad1