jadami10 commented on a change in pull request #7961:
URL: https://github.com/apache/pinot/pull/7961#discussion_r777840740



##########
File path: 
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
##########
@@ -374,6 +423,15 @@ public void start()
     LOGGER.info("Starting Pinot server");
     long startTimeMs = System.currentTimeMillis();
 
+    if (_serverConf.getProperty(Server.CONFIG_OF_STARTUP_ENABLE_TEMP_CLEANUP,
+        Server.DEFAULT_STARTUP_ENABLE_TEMP_CLEANUP)) {
+      File dataDir = new 
File(_serverConf.getProperty(CommonConstants.Server.CONFIG_OF_INSTANCE_DATA_DIR));
+      // We use 3 hours as the cutoff time as a general heuristic for when

Review comment:
       > Not a big fan of introducing a config just for cleaning up temp 
directory but I'm fine with having a fixed threshold just as you mentioned: 
delete for more than N hours. We shouldn't see multiple servers running on the 
same machine in produciton.
   
   @jackjlli preferred it this way, and I'm inclined to agree. There's already 
a large number of configs, and ideally we remove this config in the future and 
make this default behavior on server startup.

##########
File path: 
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
##########
@@ -363,6 +377,41 @@ private void startupServiceStatusCheck(long endTimeMs) {
         ServiceStatus.getStatusDescription());
   }
 
+  /**
+   * Recursively deletes all data directories starting with tmp- last modified 
before the startTime.
+   * @param startTime start time of the application
+   * @param dataDir data directory to start from
+   */
+  @VisibleForTesting
+  public static void deleteTempFilesSinceCutoffTime(long startTime, @Nonnull 
File dataDir) {
+    if (!dataDir.exists() || !dataDir.isDirectory()) {
+      LOGGER.warn("Data directory {} does not exist or is not a directory", 
dataDir);
+      return;
+    }
+    IOFileFilter beforeStartTimeFilter = new AgeFileFilter(startTime, true);
+    IOFileFilter tmpPrefixFilter = new PrefixFileFilter("tmp-");

Review comment:
       it could, but that felt like a better and easy addition for a future PR 
that requires it




-- 
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: commits-unsubscr...@pinot.apache.org

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

Reply via email to