[GitHub] [storm] Ethanlm commented on a change in pull request #3336: [STORM-3701] clean-up topo directory with the check against latest topo blob cache

[GitBox <gi...@apache.org>]

Ethanlm commented on a change in pull request #3336:
URL: https://github.com/apache/storm/pull/3336#discussion_r490986544



##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -641,7 +642,12 @@ void cleanup() {
 
             try {
                 forEachTopologyDistDir((p, topologyId) -> {
-                    if (!safeTopologyIds.contains(topologyId)) {
+                    String topoJarKey = ConfigUtils.masterStormJarKey(topologyId);
+                    String topoCodeKey = ConfigUtils.masterStormCodeKey(topologyId);
+                    String topoConfKey = ConfigUtils.masterStormConfKey(topologyId);
+                    if (!topologyBlobs.containsKey(topoJarKey)

Review comment:
       When that happens, the directory will not be deleted. It is okay because when `getTopoJar` executes, it means the directory is needed. 
   
   The bottom line here is the code will delete the directory for dead topologies eventually (cleanup thread runs every 30s). But if there is anywhere that needs this directory, this code will not delete the directory
   
   




----------------------------------------------------------------
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:
users@infra.apache.org