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



##########
File path: 
storm-server/src/main/java/org/apache/storm/utils/ServerConfigUtils.java
##########
@@ -158,4 +158,9 @@ public LocalState supervisorStateImpl(Map<String, Object> 
conf) throws IOExcepti
     public LocalState nimbusTopoHistoryStateImpl(Map<String, Object> conf) 
throws IOException {
         return new LocalState((masterLocalDir(conf) + FILE_SEPARATOR + 
"history"), true);
     }
+
+    public static long getLocalizerUpdateBlobInterval(Map<String, Object> 
conf) {
+        return ObjectReader.getInt(conf.get(

Review comment:
       This looks a little weird on how it switches between integer and long. 
It can be just integer.
   
https://github.com/apache/storm/blob/6e3707242e0e6835085d358bec0abee4f62b1426/storm-server/src/main/java/org/apache/storm/DaemonConfig.java#L769-L770
   

##########
File path: 
external/storm-hdfs-blobstore/src/main/java/org/apache/storm/hdfs/blobstore/HdfsBlobStoreImpl.java
##########
@@ -312,4 +313,54 @@ public void shutdown() {
             timer.cancel();
         }
     }
+
+    /**
+     * Get the last modification time of any blob.
+     *
+     * @return the last modification time of blobs within the blobstore.
+     * @throws IOException on any error
+     */
+    public long getLastModTime() throws IOException {
+        long modtime =  
fileSystem.getFileStatus(fullPath).getModificationTime();

Review comment:
       nit: space

##########
File path: 
external/storm-hdfs-blobstore/src/main/java/org/apache/storm/hdfs/blobstore/HdfsBlobStoreImpl.java
##########
@@ -312,4 +313,54 @@ public void shutdown() {
             timer.cancel();
         }
     }
+
+    /**
+     * Get the last modification time of any blob.
+     *
+     * @return the last modification time of blobs within the blobstore.
+     * @throws IOException on any error
+     */
+    public long getLastModTime() throws IOException {
+        long modtime =  
fileSystem.getFileStatus(fullPath).getModificationTime();
+        return modtime;
+    }
+
+    /**
+     * Updates the modification time of the blobstore to the current time.
+     *
+     * @throws IOException on any error
+     */
+    public void updateLastModTime() throws IOException {
+        long timestamp = Time.currentTimeMillis();
+        fileSystem.setTimes(fullPath, timestamp, timestamp);
+        LOG.debug("Updated blobstore modtime of {} to {}", fullPath, 
timestamp);
+    }
+
+    /**
+     * Validates that the modification time of the blobstore is up to date 
with the current existing blobs.
+     *
+     * @throws IOException on any error
+     */
+    public void validateModTime() throws IOException {
+        int currentBucket = 0;
+        long baseModTime = 0;
+        while (currentBucket < BUCKETS) {
+            String name = String.valueOf(currentBucket);
+            Path bucketDir = new Path(fullPath, name);
+
+            // only consider bucket dirs that exist with files in them
+            if (fileSystem.exists(bucketDir) && 
fileSystem.listStatus(bucketDir).length > 0) {
+                long modtime = 
fileSystem.getFileStatus(bucketDir).getModificationTime();

Review comment:
       This looks at the mod time of the bucketDir.  But the blob files can be 
updated. 
   
https://git.vzbuilders.com/storm/storm/blob/master/docs/distcache-blobstore.md#updating-a-cached-file
   
   Will this work properly in this case? Or is the code change at Nimbus.java 
serve the purpose?
   

##########
File path: 
external/storm-hdfs-blobstore/src/main/java/org/apache/storm/hdfs/blobstore/HdfsBlobStoreImpl.java
##########
@@ -312,4 +313,54 @@ public void shutdown() {
             timer.cancel();
         }
     }
+
+    /**
+     * Get the last modification time of any blob.
+     *
+     * @return the last modification time of blobs within the blobstore.
+     * @throws IOException on any error
+     */
+    public long getLastModTime() throws IOException {
+        long modtime =  
fileSystem.getFileStatus(fullPath).getModificationTime();
+        return modtime;
+    }
+
+    /**
+     * Updates the modification time of the blobstore to the current time.
+     *
+     * @throws IOException on any error
+     */
+    public void updateLastModTime() throws IOException {
+        long timestamp = Time.currentTimeMillis();
+        fileSystem.setTimes(fullPath, timestamp, timestamp);

Review comment:
       I am not very sure if changing the mtime of the blobstore directory is a 
good idea. It breaks the semantics of modification time of a 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:
us...@infra.apache.org


Reply via email to