Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/blocks should allow fetching of multiple blocks #460

Closed
Swader opened this issue Mar 4, 2021 · 7 comments · Fixed by #954
Closed

/blocks should allow fetching of multiple blocks #460

Swader opened this issue Mar 4, 2021 · 7 comments · Fixed by #954
Labels
I7 - Optimization 🚴 Make Sidecar drive faster I8 - Enhancement Additional feature request P7 - Nice to Have Nice, but not urgent

Comments

@Swader
Copy link

Swader commented Mar 4, 2021

Right now the blocks endpoint (somewhat misleadingly) fetches info for only a single block. It would be good if it supported a comma or plus separated list of blocks, and return them all in an array.

/blocks/500001+500002
or
/blocks/500001,500002

The BC-approach would be to only return as array when multiple blocks are passed in. The right approach would be a mid version bump due to a change in return values which should now ALWAYS return arrays, even when looking for a simple blocks. The single-element return should be renamed to /block and should under the hood just call /blocks/xxxx[0]

@dvdplm
Copy link
Contributor

dvdplm commented Mar 4, 2021

Why do you think /blocks/BLOCKNUM is misleading? It's a pretty standard REST-style url like e.g. /posts/123 to designate a Post object with id 123.

Renaming /blocks to /block is unlikely to happen.

I do like the idea of supporting a batch of blocks though.

@Swader
Copy link
Author

Swader commented Mar 4, 2021

To me blocks implies plural but I agree if you're thinking in REST terms.

@TarikGul
Copy link
Member

TarikGul commented Mar 4, 2021

I do like the idea of being able to query a batch of blocks as well. It would take a little planning, and design but I think it is very doable if done right.

@emostov
Copy link
Contributor

emostov commented Mar 4, 2021

To build on what David is saying: /blocks is a RESTful representation of a collection resource. /blocks/{blockId} is a single resource that belongs-to the collection resource; the current endpoint design advances this model. Since this is explicitly a RESTful service I don't see any problems in how that model is surfaced through api-sidecar's URIs.

What is the use case for a sub-collection of blocks query?

Why is that use case not currently supported by the available endpoints?

@TarikGul
Copy link
Member

TarikGul commented Mar 5, 2021

I think more so than anything this would be looked at as an optimization.

Potentially, a batch endpoint could prevent bottlenecks and increase performance for very large queries. With the upcoming benchmarking too I think it will also give us more insight on the potential capabilities of something like this.

I know that batching is usually associated with making Patch/Post requests for updating/creating a set of any items. But I think in this case for a /GET route it could reduce network overhead, especially if RPC calls to the chain could be made via batches as well.

@Swader
Copy link
Author

Swader commented Mar 5, 2021

Exactly this. We have a use case where we have a target of a few dozen blocks at a time, maybe up to a thousand. Being able to define a block range (50000-50550) or multiple blocks (50000,50001,50002) would save us a lot of time and effort in parsing, processing, and ultimately, in stressing the server which is running both the node and the sidecar instance that's connecting to it.

@emostov emostov added I7 - Optimization 🚴 Make Sidecar drive faster I8 - Enhancement Additional feature request P7 - Nice to Have Nice, but not urgent labels Mar 16, 2021
@emostov
Copy link
Contributor

emostov commented Mar 16, 2021

My proposal is that once Tarik establishes some good baselines for benchmarking and we have investigated the low hanging fruit for optimization of the /blocks/{blockId} endpoint, we can then look into add this functionality.

Ideally, prior to merging, we would be able to show this feature has performance benefits over requesting the equivalent set of blocks individually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I7 - Optimization 🚴 Make Sidecar drive faster I8 - Enhancement Additional feature request P7 - Nice to Have Nice, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants