paul-rogers commented on PR #13798: URL: https://github.com/apache/druid/pull/13798#issuecomment-1435369060
@imply-cheddar, thanks for your comments. On your four points: > 1. The structuring should cause the developer writing the message to think about their intended audience and explicitly craft a message for that audience. This is a good idea, but I'm not sure how a code framework can encourage a developer to think about how to write a message. The error types can be seen to help: if something is a `USER` error, the audience is the end user. if it is `SYSTEM`, it is the end user who will report the error to a technical person, so the audience is a support person or Druid developer. Beyond that, any suggestions how we use code structure to get a human to think about other humans? > 2. The context should be augmentative, not required. Messages themselves need to be self-contained. Agreed. That is how the class is structured. Having items be in the context allows error messages to be succinct, with the context providing extra information. This is important for sanitizing. It is much easier to mask unwanted context than to rewrite messages that reveal internal details. Since you ask that the message contain all details, do you have a suggestion for how we can do that _and_ allow a consumer to edit those message to remove unwanted details? > 3. There is a convention in druid of encasing all interpolated values in a [], it's clear that the SQL code has deviated from that in various places, it would be nice to take this chance to return to it in any place that this touches The convention is actually `word[%s]` so you get `datasource[foo]`. At a the least, there should be a space: `datasource [foo]`. Now, let's discuss the convention. First, English does not normally use brackets for quoting, so single quotes read better for end users: `datasource 'foo'`. Second, the demarkation of a value is necessary only if the value could contain spaces. That is `datasource name 'bob is not valid' is not valid` benefits from quotes. But, if a symbol does not have spaces, the message is more human-like without the quotes or brackets: `datasource foo not found` rather than `datasource [foo] not found`. Finally, there can be no confusion for numbers. So saying `Could not delete [2] segments` is just noise, the more readable choice is `Could not delete 2 segments`. No one will be confused about the number of segments in this case. So, I would suggest that we take this opportunity to revise our brackets convention to be more focused on style for end users. If we want to use brackets in log messages, that is fine. > 4. We shouldn't intentionally generate new-lines in logs. When we log out the message with context, the context should be appended to the message rather than pushed into new-lines. That's easy enough. I'll make the change. -- 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]
