Enable Error Prone check: FormatStringAnnotation#9512
Enable Error Prone check: FormatStringAnnotation#9512findepi merged 4 commits intotrinodb:masterfrom
Conversation
There was a problem hiding this comment.
Can we handle the logger based changes in a separate commit ? The one where exception should be first and followed by message
There was a problem hiding this comment.
I split it into four commits. Thanks!
There was a problem hiding this comment.
| LOG.info("Running import for %s%n%s", target, sql); | |
| LOG.info("Running import for %s", target); | |
| LOG.info("Query %s", sql); |
Maybe
There was a problem hiding this comment.
That would be a more straightforward transformation, sure, but I think that if they are two separate logs then they may get separated in the output if there are concurrent threads. Perhaps this merging can be done in a separate commit, though.
ksobolew
force-pushed
the
kudi/enable-error-prone-check-format-strings
branch
from
733e10d to
06a284d
Compare
There was a problem hiding this comment.
let's remove the e parameter now
log.debug(e, "Error on doOpenTabl");
There was a problem hiding this comment.
This is nice. What validates (does the checkstyle for) this?
There was a problem hiding this comment.
This is Error Prone, though somewhat by accident. It sees that there's no placeholder for the exception in the format string.
plugin/trino-accumulo/src/test/java/io/trino/plugin/accumulo/AccumuloQueryRunner.java
Outdated
Show resolved
Hide resolved
core/trino-main/pom.xml
Outdated
There was a problem hiding this comment.
Don't you need to add @FormatString to airlift logger methods?
There was a problem hiding this comment.
There's already one, and that's how we can find all these issues here:
@FormatMethod
public void debug(String format, Object... args)
{
...
}There was a problem hiding this comment.
That's because of this line:
log.error(format + ": %s", ObjectArrays.concat(args, t));It kind of appends the Throwable as the last thing after the original format string. I tried to make it acceptable to Error Prone, or at least make the suppression more local, but couldn't. If you can think of a better way (or perhaps to get rid of it entirely), I'd be glad to fix it properly :)
There was a problem hiding this comment.
Please add a comment. This is fine.
In some logging frameworks the exception to print is expected to be the last format string argument, even though there's no placeholder for it. In Airlift, though, this is different, and there's an explicit exception parameter before the format string. Fortunately Error Prone can flag this, although somewhat by accident (by seeing that there's no placeholder for the exception parameter).
The logic here is that if we keep these as separate log invocations, they may not be kept together in the log output if there are concurrent threads also doing logging. As a follow-up to the previous commit. Could be a more generic transformation, probably.
ksobolew
force-pushed
the
kudi/enable-error-prone-check-format-strings
branch
2 times, most recently
from
c503e7f to
5141895
Compare
core/trino-main/src/main/java/io/trino/operator/WorkProcessorPipelineSourceOperator.java
Outdated
Show resolved
Hide resolved
.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java
Outdated
Show resolved
Hide resolved
.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java
Outdated
Show resolved
Hide resolved
This forces us to annotate all the places where we wrap or delegate logs. There's one suppression, where we intentionally add one more placeholder to the format string and Error Prone won't take it in any form. It is ERROR by default. Will be removed once we enable all checks.
ksobolew
force-pushed
the
kudi/enable-error-prone-check-format-strings
branch
from
5141895 to
2010be8
Compare
|
This is nice improvement, thanks! fyi @trinodb/maintainers |
|
Thank you! |
6 participants
It mostly catches issues related to format strings in loggers, some in very old code. There's one suppression, where we intentionally add one more placeholder to the format string and Error Prone won't take it in any form.
It is ERROR by default. Will be removed once we enable all checks.