imply-cheddar commented on PR #13884: URL: https://github.com/apache/druid/pull/13884#issuecomment-1458931783
1) The log format should have a fully interpolated message. People do view errors in logs and a terse message that is only a key and a few context values is not sufficient. It should be the fully interpolated message + all of the "context" parameters on the same line. 2) It's completely unclear which persona is being targeted by an exception message. It is important that developers and reviewers and anybody who comes to read the code can see the intended persona next to the error message. Don't put it in a base class and serialize it out on all of the RPC calls 3) We do not need to support error translation (at least not now). We need to generate better error messages. The creation of names (using Strings that could easily accidentally collide between different call points) that sit next to the actual message adds little value and doesn't actually help. Reviewers will just skip over those strings as they don't add value and it is impossible for a reviewer to fully vet that the new String created is good *and* they are prone to copy-and-paste duplication. Overall, I think it's a non-requirement at this point and we should focus instead on things that generate good error messages and perhaps go back to error translation at some point in the future if it's required after we are happy with the error messages that we generate by default. 4) Regarding Faults and creating a new class for every, single, error that ever comes. Personally, I think it's overkill. It definitely makes sense in some cases and supporting the ability to provide things through a "Fault" style class has nice things, but the overhead of creating new errors is high. This overhead is the reason that I think it's overkill, for us to get value from it, we have to be willing to make it the only way that we ever generate errors (i.e. we require a new Fault class for every error), but that's a really tall order for errors that are, e.g. just defensive validations that we expect to not actually be hit very often (if ever). We could perhaps have a world where anything targeting a User persona requires a Fault class (as it's intending to be end-user facing) and all of the Admin/Developer focused ones follow a less-strict "Builder style" exception creation scheme. Either way, if we want to support `Fault` classes, we need to determine the semantics t hat make them required and then have them 100% of the time for those semantics. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
