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

impl guide: update PVF host page; add diagrams #2579

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Dec 1, 2023

Would have been nice to have some helpful diagrams a while ago, but I just thought of it. :P
cc @eagr @jpserrat

Screenshots

Screenshot 2023-12-01 at 14 46 14

Screenshot 2023-12-01 at 14 46 45

Would have been nice to have some helpful diagrams a while ago, but I just
thought of it. :P
@eagr
Copy link
Contributor

eagr commented Dec 2, 2023

would be nice if you could also post the clips of the diagrams here :))

Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

   ┌─ node/utility/pvf-prechecker.md:11:110
   │  
11 │   This subsytem does not produce any output messages either. The subsystem will, however, send messages to the [Runtime
   │ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────^
12 │ │ API] subsystem to query for the pending PVFs and to submit votes. In addition to that, it will also communicate with
   │ ╰────^ Did you forget to define a URL for `Runtime API`?
   │  
   = hint: declare the link's URL. For example: `[Runtime API]: http://example.com/`

Would be good to fix it as well. Otherwise, looks very good!

@s0me0ne-unkn0wn
Copy link
Contributor

Probably a random place to ask, but did anybody manage to get a usable PDF of the Implementer's Guide out of mdbook? I tried the printing function and mdbook-pdf crate, but the diagrams are broken af, they're not auto-scaled, and a lot of them are cut in half.

@mrcnski
Copy link
Contributor Author

mrcnski commented Dec 3, 2023

@eagr See the "Screenshots section. 😀

@s0me0ne-unkn0wn No idea, but even in HTML mode the diagrams are a bit broken, I need to scroll a lot to see the bigger ones... And it's nice that they are generated from code inline with the docs, but they're a bit ugly haha.


pvf [label = "PVF Host"; shape = square]

pp [label = "Prepare\nPool"; shape = square]
Copy link
Contributor

@eagr eagr Dec 3, 2023

Choose a reason for hiding this comment

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

Suggested change
pp [label = "Prepare\nPool"; shape = square]
pp [label = "Prepare\nPool"; shape = square]
pq [label = "Prepare\nQueue"; shape = square]
pvf -> pq [label = "Prepare"; style = dashed]
pq -> pp [label = "Prepare"; style = dashed]

Missing the prepare queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, will fix. In my head the queue and pool do the same thing, and I'm not sure why they are separate. I think I asked at one point, and the pool was supposed to fulfill some role that is actually no longer needed, but I don't remember.

Making this diagram also had me asking why Candidate Validation sends messages to the PVF host, when we could have instead embedded the host in the subsystem. It seems a bit over-engineered, and the extra message passing is inefficient, but it's cold code so 🤷

@mrcnski mrcnski self-assigned this Dec 4, 2023
@mrcnski mrcnski added T0-node This PR/Issue is related to the topic “node”. T11-documentation This PR/Issue is related to documentation. labels Dec 4, 2023
@mrcnski mrcnski merged commit 707dbcc into master Dec 4, 2023
119 of 120 checks passed
@mrcnski mrcnski deleted the mrcnski/impl-guide-update-pvf-page branch December 4, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”. T11-documentation This PR/Issue is related to documentation.
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

4 participants