Copilot commented on code in PR #16192:
URL: https://github.com/apache/pinot/pull/16192#discussion_r2165002932
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskGenerator.java:
##########
@@ -169,8 +170,11 @@ public List<PinotTaskConfig>
generateTasks(List<TableConfig> tableConfigs) {
}
}
} catch (Exception e) {
- LOGGER.error("Unable to generate the SegmentGenerationAndPush task.
[ table configs: {}, task configs: {} ]",
- tableConfig, taskConfigs, e);
+ if (LOGGER.isErrorEnabled()) {
+ LOGGER.error(
+ "Unable to generate the SegmentGenerationAndPush task. [ table
configs: {}, task configs: {} ]",
+ new Obfuscator().toJsonString(tableConfig), new
Obfuscator().toJsonString(taskConfigs), e);
Review Comment:
Obfuscator.toJsonString is intended for Map<String, String> payloads, not a
TableConfig object; passing a TableConfig may cause a compile-time type
mismatch or unexpected behavior. Consider serializing only the config map or
using a JSON utility designed for objects.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskExecutor.java:
##########
@@ -114,7 +115,10 @@ protected SegmentZKMetadataCustomMapModifier
getSegmentZKMetadataCustomMapModifi
@Override
public Object executeTask(PinotTaskConfig pinotTaskConfig)
throws Exception {
- LOGGER.info("Executing SegmentGenerationAndPushTask with task config: {}",
pinotTaskConfig);
+ if (LOGGER.isInfoEnabled()) {
+ LOGGER.info("Executing SegmentGenerationAndPushTask with task config:
{}",
+ new Obfuscator().toJsonString(pinotTaskConfig));
Review Comment:
You're passing the entire PinotTaskConfig to Obfuscator.toJsonString; this
method likely expects a Map<String, String>. Use `pinotTaskConfig.getConfigs()`
or a JSON serializer that handles complex objects correctly.
```suggestion
new Obfuscator().toJsonString(pinotTaskConfig.getConfigs()));
```
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -73,7 +74,9 @@ protected List<SegmentConversionResult>
convert(PinotTaskConfig pinotTaskConfig,
_eventObserver.notifyProgress(pinotTaskConfig, "Converting segments: " +
numInputSegments);
String taskType = pinotTaskConfig.getTaskType();
Map<String, String> configs = pinotTaskConfig.getConfigs();
- LOGGER.info("Starting task: {} with configs: {}", taskType, configs);
+ if (LOGGER.isInfoEnabled()) {
+ LOGGER.info("Starting task: {} with configs: {}", taskType, new
Obfuscator().toJsonString(configs));
Review Comment:
[nitpick] Creating a new Obfuscator instance on every log call may be
inefficient; consider reusing a single static final Obfuscator instance for the
class.
```suggestion
LOGGER.info("Starting task: {} with configs: {}", taskType,
OBFUSCATOR.toJsonString(configs));
```
--
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]
