RuiLi8080 commented on a change in pull request #3336: URL: https://github.com/apache/storm/pull/3336#discussion_r491122961
########## 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: @agresch Technically there is a chance. But with this change it would be more unlikely to happen than before. Before this change, we compare topo dir to `safeTopologyIds` which is essentially a much older version of `topologyBlobs` snapshot since it could take about 10s for the `forEachTopologyDistDir` loop to finish. In this case, race condition is much more likely to happen since the deletion loop keep looking at the snapshot 10s ago. Now we directly check the `topologyBlobs` map right before deletion. So the `forEachTopologyDistDir` loop could be aware of the concurrent additions from slots and further prevent most of the wrongful deletion to happen. ########## 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: @agresch Technically there is a chance. But with this change it would be more unlikely to happen than before. Before this change, we compare topo dir to `safeTopologyIds` which is essentially a much older version of `topologyBlobs` snapshot since it could take about 10s for the `forEachTopologyDistDir` loop to finish. In this case, race condition is much more likely to happen since the deletion loop keep looking at the snapshot 10s ago. Now we directly check the `topologyBlobs` map right before deletion. So the `forEachTopologyDistDir` loop could be aware of the concurrent additions from slots and further prevent most of the wrongful deletion to happen. ---------------------------------------------------------------- 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