Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented Dec 2, 2021

Ref #4440

Note to reviewers:

  • focus on the match of #[fatal] and #[fatal(forward)] and the missing annotations with the previous enums NonFatal and Fatal.
  • look at the API or quirks that are required, misnomers in the trait Nested or trait Split, these are easy to fix
  • ergonomics of the fatality::fatality proc-macro

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Dec 2, 2021
@drahnr drahnr modified the milestone: v0.9.15 Dec 9, 2021
@drahnr drahnr self-assigned this Dec 16, 2021
@ordian
Copy link

ordian commented Jan 21, 2022

I like the direction where it's headed. It's be good to see it applied to one subsystem to justify the design.

@drahnr drahnr force-pushed the bernhard-fatality branch from be2b47e to 433418a Compare January 21, 2022 15:30
@drahnr drahnr added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 30, 2022
@drahnr drahnr force-pushed the bernhard-fatality branch from 9958fc4 to e156b73 Compare January 31, 2022 19:18
@drahnr drahnr marked this pull request as ready for review January 31, 2022 21:22
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 31, 2022
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Noice!

@drahnr drahnr force-pushed the bernhard-fatality branch from 6a8edf8 to adc568d Compare February 3, 2022 08:49
@drahnr drahnr mentioned this pull request Feb 3, 2022
@drahnr drahnr force-pushed the bernhard-fatality branch 2 times, most recently from f6cf4eb to 74c2a40 Compare February 22, 2022 10:34
Copy link

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall. We should do a burn-in on Versi to make sure it doesn't introduce early exits or too verbose logging.

One think I prefer about our previous version is that the Fatal, NonFatal was explicit in the naming and here we have to jump to definition to know which one is fatal. But maybe it's not a big deal.

@drahnr
Copy link
Contributor Author

drahnr commented Feb 24, 2022

Technically we can also use the generated variants JfyiError and FatalError, but I wanted to defer the implicitly named types to as little usage as possible. I have an issue (gh doesn't load for me right now) to allow specifying those explicitly.

Could you outline an instance where you think the JfyiError or FatalError should be used rather than Error?

The logging verbosity should be identical to before. If there'd be a change that should be considered a bug.

@drahnr
Copy link
Contributor Author

drahnr commented Feb 24, 2022

Deployed to versi group e for burnin and a

@drahnr
Copy link
Contributor Author

drahnr commented Feb 24, 2022

@drahnr
Copy link
Contributor Author

drahnr commented Feb 25, 2022

Did a last review pass to check the correct split between Jfyi* and Fatal* and it looks all good. Did a few small simplifications, I'll merge this then.

@drahnr
Copy link
Contributor Author

drahnr commented Feb 25, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 2b801af into master Feb 25, 2022
@paritytech-processbot paritytech-processbot bot deleted the bernhard-fatality branch February 25, 2022 17:25
@ordian ordian added the T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. label Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants