mcvsubbu commented on a change in pull request #6124:
URL: https://github.com/apache/incubator-pinot/pull/6124#discussion_r503594418
##########
File path:
pinot-minion/src/main/java/org/apache/pinot/minion/executor/PinotTaskExecutor.java
##########
@@ -26,6 +26,12 @@
*/
public interface PinotTaskExecutor {
+ /**
+ * Pre processing operations to be done at the beginning of task execution
+ */
+ default void preProcess(PinotTaskConfig pinotTaskConfig) {
Review comment:
Why not make this a boolean return, so that if the pre-process fails we
do not go further in the task execution? Not sure what helix does when we throw
exceptions from the task executor -- probably retries the task, but that may
not be good for us.
##########
File path:
pinot-minion/src/main/java/org/apache/pinot/minion/executor/BaseSingleSegmentConversionExecutor.java
##########
@@ -135,6 +137,8 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
convertedSegmentTarFile);
LOGGER.info("Done executing {} on table: {}, segment: {}", taskType,
tableNameWithType, segmentName);
+ postProcess(pinotTaskConfig);
Review comment:
please move the log line to be below postProcess, since it says "Done"
It may be useful to have a log line after each phase
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
