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
