gortiz commented on code in PR #16171:
URL: https://github.com/apache/pinot/pull/16171#discussion_r2160825997
##########
pinot-minion/src/main/java/org/apache/pinot/minion/taskfactory/TaskFactoryRegistry.java:
##########
@@ -146,7 +147,7 @@ private TaskResult runInternal(PinotTaskConfig
pinotTaskConfig) {
_eventObserver.notifyTaskStart(pinotTaskConfig);
LOGGER.info("Start running {}: {} with configs: {}",
pinotTaskConfig.getTaskType(), _taskConfig.getId(),
- pinotTaskConfig.getConfigs());
+ new Obfuscator().toJsonString(pinotTaskConfig.getConfigs()));
Review Comment:
This will obfuscate the text even if the log is not logged, which would be a
waste of resources.
In SLF4J the only way to solve the issue is to do something like:
```java
if (LOGGER.isInfoEnabled()) {
LOGGER.info(whatever, usually using the " + value +" syntax instead of {})
}
```
If instead of SLF4J, which is very old, we were using log4j2 apis to log, we
could use lambdas to log, like:
```
LOGGER.trace("Start running {}: {} with configs: {}",
pinotTaskConfig.getTaskType(), _taskConfig.getId(), () -> new
Obfuscator().toJsonString(pinotTaskConfig.getConfigs())));
```
Given the log is at info level (which is usually enabled) and the fact that
this is only called during startup, this is probably not actually problematic.
So I don't think this inefficiency is a blocker, but in general I would
recommend using the pattern above in cases like this.
--
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]
