klsince commented on code in PR #9449:
URL: https://github.com/apache/pinot/pull/9449#discussion_r978160078


##########
pinot-minion/src/main/java/org/apache/pinot/minion/event/MinionProgressObserver.java:
##########
@@ -29,57 +34,100 @@
 /**
  * A minion event observer that can track task progress status in memory.
  */
+@ThreadSafe
 public class MinionProgressObserver extends DefaultMinionEventObserver {
   private static final Logger LOGGER = 
LoggerFactory.getLogger(MinionProgressObserver.class);
+  // TODO: make this configurable
+  private static final int DEFAULT_MAX_NUM_STATUS_TO_TRACK = 128;
 
-  private static volatile long _startTs;
-  private static volatile Object _lastStatus;
+  private final int _maxNumStatusToTrack;
+  private final Deque<StatusEntry> _lastStatus = new LinkedList<>();
+  private long _startTs;
+
+  public MinionProgressObserver() {
+    this(DEFAULT_MAX_NUM_STATUS_TO_TRACK);
+  }
+
+  public MinionProgressObserver(int maxNumStatusToTrack) {
+    _maxNumStatusToTrack = maxNumStatusToTrack;
+  }
 
   @Override
-  public void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
+  public synchronized void notifyTaskStart(PinotTaskConfig pinotTaskConfig) {
     _startTs = System.currentTimeMillis();
+    addStatus(_startTs, "Task started");
     super.notifyTaskStart(pinotTaskConfig);
   }
 
-  public void notifyProgress(PinotTaskConfig pinotTaskConfig, @Nullable Object 
progress) {
+  public synchronized void notifyProgress(PinotTaskConfig pinotTaskConfig, 
@Nullable Object progress) {
     if (LOGGER.isDebugEnabled()) {
       LOGGER.debug("Update progress: {} for task: {}", progress, 
pinotTaskConfig.getTaskId());
     }
-    _lastStatus = progress;
+    addStatus(System.currentTimeMillis(), (progress == null) ? "" : 
progress.toString());

Review Comment:
   good question. not always but kept things simple for now as all status is 
String right now. I'll add a method comment that the status.toString() should 
return sth meaningful, but this is only for `MinionProgressObserver` class, as 
one can implement a different MinionEventObserver which does not do toString(). 



-- 
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]

Reply via email to