[pbs-devel] applied: [PATCH proxmox-backup v3 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 24.01.25 um 16:29 schrieb Laurențiu Leahu-Vlăducu:
> * Improved errors when panics occur and the panic message is a
> formatted (not static) string. This worked already for &str literals,
> but not for Strings.
> 
> Downcasting to both &str and String is also done by the Rust Standard
> Library in the default panic handler. See:
> https://github.com/rust-lang/rust/blob/b605c65b6eb5fa71783f8e26df69975f9f1680ee/library/std/src/panicking.rs#L777
> 
> * Switched from eprintln! to tracing::error when logging panics in the
> task scheduler.

This describes what happens but not why and which visible changes this
has, or not has, like I asked in my review for v2.

Background reasoning is important for commit messages, as while a
summary about "what" changes can be quite nice to have it can almost
always reconstructed relatively easy from checking the diff. The
semantic change that affects users and or devs OTOH is harder to
reconstruct, sometimes even impossible (besides that brains tend to
forget details, staff is not always around to be able to ask them).
And while one won't need that information for all commits, one cannot
really predict for which, so adding a rationale for every non-trivial
commit–with a rather narrow definition of trivial–is the only way to
ensure that this information is there when needed; short of time-travel
;) Note that one does not need a ton of text, one or two sentences can
often be enough, sometimes a few short paragraphs tend to help though;
I'm also fine with always going for relatively long commit messages, a
short summary at the top would be helpful then though.

Oh and FWIW, for this change it would have been fine to switch over to
tracing::error in a separate patch, as it's not directly related to
better relaying errors from panics.

The first part above I mentioned on v2, that's on you so to say, the
latter I did not mention when reviewing on v2, so that's on me, as it's
not a big change and I did not want to delay it further I now applied
the patches as is.

And the reason I went for a relatively big explanation here, which
actually applies to all devs not just you, is mostly because first,
you're relatively new at Proxmox thus a bit stricter review with more
rationale is warranted and second, doing so safes, among others, the
people writing changelogs and tracking down bugs a ton of time, and I
tend to do both quite often and know the pain about having to retrace
all relations and background reasoning from a fresh starts because the
author did not bother with any commit message, and while it was
self-explanatory or obvious back then for them, they "interestingly"
often do not remember when asking about the background in persona; which
_is_ relatable, as said making errors and forgetting things is human,
but as the fix is so easy, i.e. just write an actual commit message with
some actual background, it's nonetheless not a good excuse.  Another
benefit of writing short summarize is that one needs to reflect on the
work on a higher level, just like rubber duck programming that can help
to improve things without another's person's review or sometimes also
surface some flaws.

anyhow: applied, thanks!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel