[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #13649: [Feature-13419][Remote Logging] Add support for writing task logs to S3

["Radeity (via GitHub)" <gi...@apache.org>]

Radeity commented on code in PR #13649:
URL: https://github.com/apache/dolphinscheduler/pull/13649#discussion_r1132163969


##########
dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java:
##########
@@ -21,19 +21,25 @@
 import org.apache.dolphinscheduler.common.utils.PropertyUtils;
 
 import lombok.experimental.UtilityClass;
+import lombok.extern.slf4j.Slf4j;
 
 @UtilityClass
+@Slf4j
 public class RemoteLogHandlerFactory {
 
     public RemoteLogHandler getRemoteLogHandler() {
         if (!RemoteLogUtils.isRemoteLoggingEnable()) {
             return null;
         }
-        if (!"OSS".equals(PropertyUtils.getUpperCaseString(Constants.REMOTE_LOGGING_TARGET))) {
-            return null;
+
+        String target = PropertyUtils.getUpperCaseString(Constants.REMOTE_LOGGING_TARGET);
+        if ("OSS".equals(target)) {
+            return OssRemoteLogHandler.getInstance();
+        } else if ("S3".equals(target)) {
+            return S3RemoteLogHandler.getInstance();
         }
-        OssRemoteLogHandler ossRemoteLogHandler = new OssRemoteLogHandler();
-        ossRemoteLogHandler.init();
-        return ossRemoteLogHandler;
+
+        log.error("No suitable remote logging target for {}", target);

Review Comment:
   Some tiny suggestion, if we write an error log here, do we have to also write an error log which may be close in meaning when judging `remoteLogHandler == null`?
   ```java
   if (remoteLogHandler == null) {
       log.error("remote log handler is null");
       return;
   }
   ```



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