Add pagination and timeout to commitments and ack queries #4110

ljoss17 · 2024-07-31T09:35:12Z

Closes: #4101

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

…ments

romac · 2024-08-07T09:40:37Z

.changelog/unreleased/improvements/ibc-relayer/4101-add-pagination.md

+- Implemented pagination for `query_packet_commitments` and `query_packet_acknowledgements`
+  during the scanning phase to optimize the determination of whether to spawn a packet worker.


Suggested change

- Implemented pagination for `query_packet_commitments` and `query_packet_acknowledgements`

during the scanning phase to optimize the determination of whether to spawn a packet worker.

- Paginate results of `query_packet_commitments` and `query_packet_acknowledgements`

queries to speed up the scanning phase.

romac · 2024-08-07T09:40:53Z

crates/relayer-cli/src/commands/query/packet/pending.rs

@@ -1,6 +1,7 @@
 use core::fmt;

 use abscissa_core::clap::Parser;
+use ibc_relayer::chain::requests::Paginate;


Let's move this in the import group below.

romac · 2024-08-07T09:43:53Z

crates/relayer/src/chain/cosmos.rs

-            .and_then(|raw_height| raw_height.try_into().ok())
-            .ok_or_else(|| Error::grpc_response_param("height".to_string()))?;
+                let mut tonic_request = tonic::Request::new(raw_request);
+                tonic_request.set_timeout(Duration::from_secs(10));


Just a note for us that we should eventually make this either:
a) Configurable (eg. grpc_timeout setting)
b) Inferred from the pagination, ie. use larger timeout for Paginate::All and smaller one otherwise

romac · 2024-08-07T09:44:45Z

crates/relayer/src/chain/cosmos.rs

+
+            let responses =
+                results.into_iter().collect::<Result<
+                    Vec<ibc_proto::ibc::core::channel::v1::QueryPacketCommitmentsResponse>,


Can't the inner type be elided here?

Suggested change

Vec<ibc_proto::ibc::core::channel::v1::QueryPacketCommitmentsResponse>,

Vec<_>,

romac · 2024-08-07T09:45:14Z

crates/relayer/src/chain/cosmos.rs

-            .map(|v| v.sequence.into())
-            .collect();
+                let mut tonic_request = tonic::Request::new(raw_request);
+                tonic_request.set_timeout(Duration::from_secs(10));


romac · 2024-08-07T09:45:19Z

crates/relayer/src/chain/cosmos.rs

+            }
+
+            let responses = results.into_iter().collect::<Result<
+                Vec<ibc_proto::ibc::core::channel::v1::QueryPacketAcknowledgementsResponse>,


romac · 2024-08-07T09:46:30Z

crates/relayer/src/chain/counterparty.rs

 ) -> Result<Option<(Vec<Sequence>, Height)>, Error> {
-    let (commitments_on_src, _) = commitments_on_chain(chain, &path.port_id, &path.channel_id)?;
+    let (commitments_on_src, _) =
+        commitments_on_chain(chain, &path.port_id, &path.channel_id, pagination.clone())?;


Paginate can be made Copy:

Suggested change

commitments_on_chain(chain, &path.port_id, &path.channel_id, pagination.clone())?;

commitments_on_chain(chain, &path.port_id, &path.channel_id, pagination)?;

romac · 2024-08-07T09:47:05Z

crates/relayer/src/chain/requests.rs

+        if let Paginate::PerPage {
+            pagination: _,
+            limit,
+        } = self
+        {
+            return *limit;
+        }
+        0


A simple match would look more idiomatic here

romac · 2024-08-07T09:47:40Z

crates/relayer/src/chain/requests.rs

+        match value {
+            Paginate::All => PageRequest::all(),
+            Paginate::PerPage {
+                pagination: _,
+                limit,
+            } => PageRequest::per_page(limit),
+        }


Suggested change

match value {

Paginate::All => PageRequest::all(),

Paginate::PerPage {

pagination: _,

limit,

} => PageRequest::per_page(limit),

}

match value {

Paginate::All => PageRequest::all(),

Paginate::PerPage { limit, .. } => PageRequest::per_page(limit),

}

romac · 2024-08-07T09:51:01Z

crates/relayer/src/chain/requests.rs

+    All,
+
+    PerPage {
+        pagination: u64,


Is this field ever used?

This is a mistake in when converting to the PageRequest. The pagination should be the limit, number of results, and limit should be the number of times the query is done. I will update this code

hermes/crates/relayer/src/chain/requests.rs

Line 176 in ead830b

} => PageRequest::per_page(limit),

to use pagination

I will rename to:

PerPage { limit: u64, max_queries: u64, },

imho it should contain two fields: one which specifies how many results per page we want and one which specifies how many results in total we want.

So maybe per_page: u64 and total: u64?

then we can just stop issuing queries once we either a) have total results or b) there is no more next page/key

Oh I had it set to one field for how many results per page and another one for the maximum number of queries. I will change to the option you suggested

romac · 2024-08-07T09:53:58Z

crates/relayer/src/chain/cosmos.rs

            .map_err(Error::grpc_transport)?;

-        client = client
-            .max_decoding_message_size(self.config().max_grpc_decoding_size.get_bytes() as usize);
+        if request.pagination.is_enabled() {


Since we are dealing with pagination in two queries, and are duplicating a bunch of fairly complex code, how about extracting the pagination logic in a standalone function we can use in both places?

async fn paginate<C, Req, Res, F>( mut client: C, request: Req, mut do_query: impl FnMut(&mut C, Req, Vec<u8>) -> F, get_next_key: impl Fn(&Res) -> Option<Vec<u8>>, ) -> Vec<Result<Res, Error>> where F: Future<Output = Result<Res, Error>> + 'static, Req: Clone, { let mut results = vec![]; let mut page_key = vec![]; loop { let response = do_query(&mut client, request.clone(), std::mem::take(&mut page_key)).await; match response { Ok(response) => { let next_key = get_next_key(&response); results.push(Ok(response)); match next_key { Some(next_key) if !next_key.is_empty() => { page_key = next_key; } _ => break, } } Err(e) => { results.push(Err(e)); break; } } } results }

We can leave this for a follow-up PR, or when we add pagination to yet another query.

ljoss17 · 2024-08-07T12:20:33Z

Applied the suggestions in this commit d048aff.
And opened this issue to follow-up on the paginate method abstraction #4135

ljoss17 added 6 commits July 31, 2024 11:34

Add pagination and timeout to query packet commitments and acknowlege…

25d0bdf

…ments

Add limit to 1 query for commitments and acks during scanning

36ff832

Add benchmarking test for query commitments

d44369b

Merge branch 'master' into luca_joss/add-pagination-to-queries

4e7d407

Fix issue with benchmark feature

d45d31a

Add changelog entry

ead830b

ljoss17 requested a review from romac August 2, 2024 12:06

ljoss17 marked this pull request as ready for review August 2, 2024 12:08

romac requested changes Aug 7, 2024

View reviewed changes

romac reviewed Aug 7, 2024

View reviewed changes

ljoss17 mentioned this pull request Aug 7, 2024

Abstract pagination method in queries #4135

Open

5 tasks

ljoss17 added 2 commits August 7, 2024 12:56

Merge branch 'master' into luca_joss/add-pagination-to-queries

fa340ae

Apply github suggestions

d048aff

romac approved these changes Aug 9, 2024

View reviewed changes

ljoss17 added this pull request to the merge queue Aug 12, 2024

Merged via the queue into master with commit e099709 Aug 12, 2024
31 checks passed

ljoss17 deleted the luca_joss/add-pagination-to-queries branch August 12, 2024 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination and timeout to commitments and ack queries #4110

Add pagination and timeout to commitments and ack queries #4110

ljoss17 commented Jul 31, 2024 •

edited

Loading

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024 •

edited

Loading

ljoss17 Aug 7, 2024

ljoss17 Aug 7, 2024

romac Aug 7, 2024 •

edited

Loading

romac Aug 7, 2024

romac Aug 7, 2024

ljoss17 Aug 7, 2024

romac Aug 7, 2024

romac Aug 7, 2024

ljoss17 commented Aug 7, 2024

		- Implemented pagination for `query_packet_commitments` and `query_packet_acknowledgements`
		during the scanning phase to optimize the determination of whether to spawn a packet worker.

	Vec<ibc_proto::ibc::core::channel::v1::QueryPacketCommitmentsResponse>,
	Vec<_>,

	commitments_on_chain(chain, &path.port_id, &path.channel_id, pagination.clone())?;
	commitments_on_chain(chain, &path.port_id, &path.channel_id, pagination)?;

Add pagination and timeout to commitments and ack queries #4110

Add pagination and timeout to commitments and ack queries #4110

Conversation

ljoss17 commented Jul 31, 2024 • edited Loading

Description

PR author checklist:

Reviewer checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljoss17 commented Aug 7, 2024

ljoss17 commented Jul 31, 2024 •

edited

Loading

romac Aug 7, 2024 •

edited

Loading

romac Aug 7, 2024 •

edited

Loading