[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #10265: [Feature][Resource] Add local resource system support in configuration resource.storage.type=HDFS_LOCAL #9385

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

zhongjiajie commented on code in PR #10265:
URL: https://github.com/apache/dolphinscheduler/pull/10265#discussion_r884469305


##########
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/config/StoreConfiguration.java:
##########
@@ -43,6 +46,12 @@ public StorageOperate storageOperate() {
         switch (PropertyUtils.getString(RESOURCE_STORAGE_TYPE)) {
             case STORAGE_S3:
                 return S3Utils.getInstance();
+            case STORAGE_LOCAL:
+                // override the FS_DEFAULT_FS configuration
+                PropertyUtils.setValue(FS_DEFAULT_FS,STORAGE_LOCAL_DEFAULT_FS);

Review Comment:
   LGTM overall, but set PropertyUtils in StoreConfiguration is a litter odd IMO, If I do this job, I will add some callback func or a hook in https://github.com/apache/dolphinscheduler/blob/996790ce9ea48bd96aed6687d31afe931fc70893/dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/PropertyUtils.java#L65 after all property set. 



##########
dolphinscheduler-common/src/main/resources/common.properties:
##########
@@ -18,7 +18,7 @@
 # user data local directory path, please make sure the directory exists and have read write permissions
 data.basedir.path=/tmp/dolphinscheduler
 
-# resource storage type: HDFS, S3, NONE
+# resource storage type: HDFS, HDFS_LOCAL, S3, NONE

Review Comment:
   I think we should directly use `LOCAL` instead of `HDFS_LOCAL` here, It is more easier to understand



-- 
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: commits-unsubscribe@dolphinscheduler.apache.org

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