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

Allow querying and returning multiple items at once in chainHead_unstable_storage #51

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 19, 2023

Close #48
Close #47

This complicates the API quite a bit, and a lot of subtle details needed to change in order for this to work:

  • In the case of a closest descendant Merkle value, we must now provide the original key in order to avoid ambiguities when you request the closest descendant Merkle value of multiple items at the same time.
  • No error is generated anymore if the trie is empty and you request the closest descendant Merkle value. Instead there's no result now. This complicates a bit the JSON-RPC client code (which is why an error was thrown originally) but is necessary for multiple items to work.
  • The server is allowed to return duplicate information, because it might not be possible for the server to de-duplicate information without using tons of memory, while the JSON-RPC client can just filter out whatever it already has.
  • The server is free to merge multiple items in the result if it so desired, or leave them split.

@tomaka tomaka changed the title Allow multiple items in chainHead_unstable_storage Allow querying and returning multiple items in chainHead_unstable_storage Jun 19, 2023
@tomaka tomaka changed the title Allow querying and returning multiple items in chainHead_unstable_storage Allow querying and returning multiple items at once in chainHead_unstable_storage Jun 19, 2023
tomaka

This comment was marked as outdated.

@josepot
Copy link
Contributor

josepot commented Jun 19, 2023

Close #46 Close #47

It actually closes: #47 and #48

If items contains multiple identical or overlapping queries, the JSON-RPC server can choose whether to merge or not the items in the result.

From the consumer point of view: the ideal API would be such that it would return an Object (rather than returning an Array) where the keys of this Object would be the different keys that were passed on the items argument, and thus always merging the results on that item.

However, if this makes the implementation of the JSON-RPC server even more complex and/or unperformant, then it's not a big deal, of course, since we can handle that on the client-side.

@tomaka
Copy link
Contributor Author

tomaka commented Jun 19, 2023

thus always merging the results on that item

That means that the server might have to wait for some of the results to have come back.
For example, if you ask at the same time for the hashes of the descendants of 0xaa and for the value of 0xaabb, the server might find the value of 0xaabb before it has found the descendants of 0xaa. If it was forced to merge the results, it would need to keep the value of 0xaabb on the side for later instead of sending it back immediately.

@josepot
Copy link
Contributor

josepot commented Jun 19, 2023

That means that the server might have to wait for some of the results to have come back.
For example, if you ask at the same time for the hashes of the descendants of 0xaa and for the value of 0xaabb, the server might find the value of 0xaabb before it has found the descendants of 0xaa. If it was forced to merge the results, it would need to keep the value of 0xaabb on the side for later instead of sending it back immediately.

I'm afraid that I didn't explain myself very well. With what I'm proposing the server wouldn't have to wait, no. It would just send an Object with what it has at the moment. If later it receives more data, then it can send another items notification with that data (even if that key has already been seen before). As long as once it's done sending all the data, it then sends the done notification, then I don't see why it should have to wait.

But again, if this means a lot of headaches on the server-side, then the API that you are suggesting is perfectly fine, for sure! This is just a "nice to have" from the consumer standpoint, not a big deal.

@josepot
Copy link
Contributor

josepot commented Jun 20, 2023

So, @tomaka: what do you think about emitting a dictionary, rather than emitting an list? Does it make sense? Does it make the spec and/or the implementation on the server too complex?

Once again: that suggestion was just a "it would be even nicer, if...". So, if you are on the fence, then let's go with what's in this PR. The last thing that I would want is for this PR to become stale. Please let me know what's your preference. Thanks!

@tomaka
Copy link
Contributor Author

tomaka commented Jun 20, 2023

The server can accommodate both solutions, because if sending an object is too annoying it can always bypass the problem by sending multiple items notifications.

However, if the server wants to generate as few items notifications as possible, then having to always merge the result forces it to buffer and organize the items that it already has.

In other words, the best possible server-side performance can only be achieved if you return a list.

@josepot josepot self-requested a review June 20, 2023 10:49
josepot
josepot previously approved these changes Jun 20, 2023
Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

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

@tomaka thank you so much for listening to the feedback and for acting so promptly on it. From the side of someone who is trying to create a JSON-RPC client based on this spec: I have to say that I'm super happy with these changes.

I will let @lexnv merge this, in case that he has some questions or reservations on his end.

@josepot josepot requested a review from lexnv June 20, 2023 11:04
lexnv
lexnv previously approved these changes Jun 22, 2023
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for extending this 🙏

@tomaka tomaka dismissed stale reviews from lexnv and josepot via 096acc6 June 22, 2023 16:39
@josepot josepot merged commit bea1a60 into paritytech:main Jun 23, 2023
@tomaka tomaka deleted the multistorage branch June 23, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing ability to batch storage requests RFC: add items notification for chainHead_unstable_storage
3 participants