[GitHub] [hadoop] mukund-thakur commented on a diff in pull request #4730: HADOOP-18371. s3a FS init logs at warn if fs.s3a.create.storage.class is unset

[GitBox]

mukund-thakur commented on code in PR #4730:
URL: https://github.com/apache/hadoop/pull/4730#discussion_r946008152


##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -997,13 +997,17 @@ protected RequestFactory createRequestFactory() {
     String storageClassConf = getConf()
         .getTrimmed(STORAGE_CLASS, "")
         .toUpperCase(Locale.US);
-    StorageClass storageClass;
-    try {
-      storageClass = StorageClass.fromValue(storageClassConf);
-    } catch (IllegalArgumentException e) {
-      LOG.warn("Unknown storage class property {}: {}; falling back to default 
storage class",
-          STORAGE_CLASS, storageClassConf);
-      storageClass = null;
+    StorageClass storageClass = null;
+    if (!storageClassConf.isEmpty()) {
+      try {
+        storageClass = StorageClass.fromValue(storageClassConf);
+      } catch (IllegalArgumentException e) {
+        LOG.warn("Unknown storage class property {}: {}; falling back to 
default storage class",
+            STORAGE_CLASS, storageClassConf);
+      }
+    } else {
+      LOG.info("Empty storage class property {}; falling back to default 
storage class",

Review Comment:
   Also shouldn't the log message say " Unset" rather than empty?



##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -997,13 +997,17 @@ protected RequestFactory createRequestFactory() {
     String storageClassConf = getConf()
         .getTrimmed(STORAGE_CLASS, "")
         .toUpperCase(Locale.US);
-    StorageClass storageClass;
-    try {
-      storageClass = StorageClass.fromValue(storageClassConf);
-    } catch (IllegalArgumentException e) {
-      LOG.warn("Unknown storage class property {}: {}; falling back to default 
storage class",
-          STORAGE_CLASS, storageClassConf);
-      storageClass = null;
+    StorageClass storageClass = null;
+    if (!storageClassConf.isEmpty()) {
+      try {
+        storageClass = StorageClass.fromValue(storageClassConf);
+      } catch (IllegalArgumentException e) {
+        LOG.warn("Unknown storage class property {}: {}; falling back to 
default storage class",
+            STORAGE_CLASS, storageClassConf);
+      }
+    } else {
+      LOG.info("Empty storage class property {}; falling back to default 
storage class",

Review Comment:
   We print configuration related logs in debug only unless they are supposed 
to cause error. So I think this should be debug. right @steveloughran 



-- 
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: common-issues-unsubscr...@hadoop.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org