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

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

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



##########
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:
       What if there is no `<redeployTime>` tag in the file? Is this always supposed to be there? 
   
   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.
   I understand if the change comes from outside (from knoxcli) this is needed, but here we are inside knox so theoretically we should be able to just call the code that does the redeployment.
   
   I also wonder if it would make sense to make redeployTime as an attribute of the root tag for example, making it less apparent.




-- 
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