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

update PAPI and improve fellows-action #20

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

josepot
Copy link
Contributor

@josepot josepot commented Mar 30, 2024

It fixes this issue, significantly simplifies the code-base, and improves its performance (by paralellizing the identity requests).

A few clarifications:

  • Starting the 2 clients in parallel is preferable than starting them sequentially, and there is no performance overhead for doing so.
  • I think that the issue that happened here, in fact has to do with this. However, that error shouldn't have happened... So, we are looking into that. We actually think that it's possible that the error being logged is actually masking an error with smoldot, but we haven't gotten to the bottom of that yet.
  • We have put a lot of effort to make sure that the consumers of PAPI can parallelize their storage requests. The library internally handles the complexities of ensuring that if all the slots guaranteed by the JSON-RPC spec are taken, that the requests are queued optimally... So, please, use that feature and avoid making sequential requests for things that could be parallelized 🙂.
  • The only thing that needs to be done inside the finally clause is to kill smoldot, because that will also effectively kill all the clients if they are still active.

@josepot josepot requested a review from a team as a code owner March 30, 2024 00:34
@josepot josepot force-pushed the feat/improve-fellows-action branch 4 times, most recently from 10e0c3a to 5927315 Compare March 30, 2024 15:45
@josepot josepot force-pushed the feat/improve-fellows-action branch from 5927315 to 4e7975f Compare March 30, 2024 15:53
Copy link

@rzadp rzadp left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I don't understand one thing though:

Before, we had two smoldot.addChain() invocations - one for polkadot and one for polkadot_collectives.

Now, there is only one - for polkadot.

What's up with that?

@rzadp rzadp requested a review from a team April 2, 2024 08:03
@josepot
Copy link
Contributor Author

josepot commented Apr 2, 2024

Looks good to me.

I don't understand one thing though:

Before, we had two smoldot.addChain() invocations - one for polkadot and one for polkadot_collectives.

Now, there is only one - for polkadot.

What's up with that?

Well, as you can see, this is completely bogus. It's just adding a chain for the sake of it, which is not being used anywhere... 🤷

@rzadp
Copy link

rzadp commented Apr 2, 2024

It's just adding a chain for the sake of it

Yeah but you still have to add it so it can sync, right?

I took a look and see that smoldot.addChain exists inside getSmProvider, so it makes sense to me now.

@rzadp rzadp merged commit 451080c into paritytech:main Apr 2, 2024
8 checks passed
@josepot josepot deleted the feat/improve-fellows-action branch April 2, 2024 08:28
@Bullrich Bullrich mentioned this pull request Apr 3, 2024
Bullrich added a commit that referenced this pull request Apr 3, 2024
As per [the
docs](https://github.com/paritytech/get-fellows-action?tab=readme-ov-file#deployment),
unless we update those two files, the new version will not be deployed
as a Docker package.

Required to have #20's change.

Also, we need this to be merged before polkadot-fellows/runtimes#265,
else it will not work as the package doesn't currently exist.
Bullrich added a commit that referenced this pull request Apr 3, 2024
Added logs that were removed during #20

This logs are necessary because, if the action is failing with a specific user, having their id can be very helpful at debugging.
@Bullrich Bullrich mentioned this pull request Apr 3, 2024
Bullrich added a commit that referenced this pull request Apr 4, 2024
Added logs that were removed during #20

This logs are necessary because, if the action is failing with a
specific user, having their address can be very helpful at debugging.
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.

None yet

2 participants