[GitHub] [knox] smolnar82 commented on a change in pull request #518: KNOX-2689 - Redeploying a topology only if it was actually changed

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

smolnar82 commented on a change in pull request #518:
URL: https://github.com/apache/knox/pull/518#discussion_r750099023



##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
##########
@@ -179,32 +182,21 @@ private void redeployTopology(Topology topology) {
         }
       }
 
-      long start = System.currentTimeMillis();
-      long limit = 1000L; // One second.
-      long elapsed = 1;
-      while (elapsed <= limit) {
-        try {
-          long origTimestamp = topologyFile.lastModified();
-          long setTimestamp = Math.max(System.currentTimeMillis(), topologyFile.lastModified() + elapsed);
-          if(topologyFile.setLastModified(setTimestamp)) {
-            long newTimstamp = topologyFile.lastModified();
-            if(newTimstamp > origTimestamp) {
-              break;
-            } else {
-              Thread.sleep(10);
-              elapsed = System.currentTimeMillis() - start;
-            }
-          } else {
-            auditor.audit(Action.REDEPLOY, topology.getName(), ResourceType.TOPOLOGY,
-                ActionOutcome.FAILURE);
-            log.failedToRedeployTopology(topology.getName());
-            break;
-          }
-        } catch (InterruptedException e) {
-          auditor.audit(Action.REDEPLOY, topology.getName(), ResourceType.TOPOLOGY, ActionOutcome.FAILURE);
-          log.failedToRedeployTopology(topology.getName(), e);
-          Thread.currentThread().interrupt();
-        }
+      // Since KNOX-2689, updating the topology file's timestamp is not enough.
+      // We need to make an actual change in the topology XML to redeploy it
+      // This change is: updating a new XML element called redeployTime
+      try {
+        final String currentTopologyContent = FileUtils.readFileToString(topologyFile, StandardCharsets.UTF_8);

Review comment:
       Thanks, @zeroflag for your review comments. Let me try to reply to them one by one:
   > What if there is no <redeployTime> tag in the file? Is this always supposed to be there?
   
   It's not a problem. In that case, the default of `-1` is used.
   
   > I wonder if it's possible to use some internal notification mechanism to send some event to the watcher to redeploy the topology without needing to modify the file.
   
   Unfortunately, currently, modifying the topology file is the _only_ option. Of course, we _could_ implement a framework that listens/receives such events, but it'd very fragile and - IMO - way too heavy for this operation.
   
   > I also wonder if it would make sense to make redeployTime as an attribute of the root tag for example, making it less apparent.
   
   That's a good idea and I also considered that option but I found it more convenient to implement it as a separate XML tag (following the current XML layout we have in topologies). If you, or the rest of the team, convinces me it's a must before we merge this change I'll update my changes.
   




-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org