zhongjiajie commented on code in PR #13332: URL: https://github.com/apache/dolphinscheduler/pull/13332#discussion_r1061495192
########## dolphinscheduler-common/src/main/resources/common.properties: ########## @@ -127,4 +127,19 @@ ml.mlflow.preset_repository_version="main" appId.collect=log # The default env list will be load by Shell task, e.g. /etc/profile,~/.bash_profile -shell.env_source_list= \ No newline at end of file +shell.env_source_list= + +# remote logging +remote.logging.enable=false +# if remote.logging.enable = true, set the target of remote logging +remote.logging.target=OSS +# oss access key id, required if you set remote.logging.target=OSS +remote.logging.oss.access.key.id=<access.key.id> +# oss access key secret, required if you set remote.logging.target=OSS +remote.logging.oss.access.key.secret=<access.key.secret> +# oss bucket name, required if you set remote.logging.target=OSS +remote.logging.oss.bucket.name=<bucket.name> +# oss endpoint, required if you set remote.logging.target=OSS +remote.logging.oss.endpoint=<endpoint> +# oss base directory, required if you set remote.logging.target=OSS +remote.logging.oss.base.dir=logs Review Comment: not sure whether some of our remote log plugin without `base.dir`, if all of them have this config, we should use `remote.logging.base.dir` instead of `remote.logging.oss.base.dir` ########## dolphinscheduler-common/src/main/resources/common.properties: ########## @@ -127,4 +127,19 @@ ml.mlflow.preset_repository_version="main" appId.collect=log # The default env list will be load by Shell task, e.g. /etc/profile,~/.bash_profile -shell.env_source_list= \ No newline at end of file +shell.env_source_list= + +# remote logging +remote.logging.enable=false +# if remote.logging.enable = true, set the target of remote logging Review Comment: should we add a comment to tell users we support oss only currently? ########## dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java: ########## @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.dolphinscheduler.common.log.remote; + +import org.apache.dolphinscheduler.common.constants.Constants; +import org.apache.dolphinscheduler.common.utils.PropertyUtils; + +import lombok.experimental.UtilityClass; + +@UtilityClass +public class RemoteLogHandlerFactory { + + public RemoteLogHandler getRemoteLogHandler() { + if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) { Review Comment: how about use `PropertyUtils.getBoolean` directly? ########## dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java: ########## @@ -2179,4 +2184,22 @@ private enum WorkflowRunnableStatus { } + private void sendRemoteLogIfNeeded(TaskInstance taskInstance) { + if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) { + if (taskInstance.getHost().endsWith(masterAddress.split(":")[1])) { Review Comment: use `PropertyUtils.getBoolean` instead of string, and we have `COLON` in constants for `:` ########## dolphinscheduler-worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/WorkerTaskExecuteRunnable.java: ########## @@ -279,6 +284,25 @@ protected void sendTaskResult() { taskExecutionContext.getCurrentExecutionStatus()); } + protected void sendRemoteLogIfNeeded() { + if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) { + try { + logger.info("Start to send log {} to remote target {}", taskExecutionContext.getLogPath(), + PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET)); + logger.info("Wait log {} to be flushed...", taskExecutionContext.getLogPath()); + Thread.sleep(5000); Review Comment: why we have to sleep for 5s here? can we run continue when we get log path immediately? ########## dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java: ########## @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.dolphinscheduler.common.log.remote; + +import org.apache.dolphinscheduler.common.constants.Constants; +import org.apache.dolphinscheduler.common.utils.PropertyUtils; + +import lombok.experimental.UtilityClass; + +@UtilityClass +public class RemoteLogHandlerFactory { + + public RemoteLogHandler getRemoteLogHandler() { + if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) { + if ("OSS".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET))) { Review Comment: we have `getUpperCaseString` method now ```suggestion if ("OSS".equals(PropertyUtils.getUpperCaseString(Constants.REMOTE_LOGGING_TARGET))) { ``` ########## dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandler.java: ########## @@ -0,0 +1,25 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.dolphinscheduler.common.log.remote; + +public interface RemoteLogHandler { + + void sendRemoteLog(String logPath); + + void getRemoteLog(String logPath); Review Comment: should we also not add an interface to test whether the current remote target work or not? ########## dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java: ########## @@ -2179,4 +2184,22 @@ private enum WorkflowRunnableStatus { } + private void sendRemoteLogIfNeeded(TaskInstance taskInstance) { + if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) { + if (taskInstance.getHost().endsWith(masterAddress.split(":")[1])) { + try { + logger.info("Start to send master's log {} to remote target {}", taskInstance.getLogPath(), + PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET)); + + RemoteLogHandler remoteLogHandler = RemoteLogHandlerFactory.getRemoteLogHandler(); + remoteLogHandler.sendRemoteLog(taskInstance.getLogPath()); + + logger.info("Succeed to send master's log {} to remote target {}", taskInstance.getLogPath(), + PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET)); + } catch (Exception e) { + logger.error("send master's log {} to remote target error", taskInstance.getLogPath(), e); Review Comment: I find out we already catch exceptions in `sendRemoteLog` method is there any expected exception will be thrown during log sending? if not I think one catch is enough, in handler or in currently ########## dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/OssRemoteLogHandler.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.dolphinscheduler.common.log.remote; + +import org.apache.dolphinscheduler.common.constants.Constants; +import org.apache.dolphinscheduler.common.factory.OssClientFactory; +import org.apache.dolphinscheduler.common.model.OssConnection; +import org.apache.dolphinscheduler.common.utils.PropertyUtils; + +import java.io.File; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.aliyun.oss.OSS; +import com.aliyun.oss.model.GetObjectRequest; +import com.aliyun.oss.model.PutObjectRequest; + +public class OssRemoteLogHandler implements RemoteLogHandler { + + private static final Logger logger = LoggerFactory.getLogger(OssRemoteLogHandler.class); + @Override + public void sendRemoteLog(String logPath) { + String accessKeyId = readOssAccessKeyId(); + String accessKeySecret = readOssAccessKeySecret(); + String endpoint = readOssEndpoint(); + OSS ossClient = OssClientFactory.buildOssClient(new OssConnection(accessKeyId, accessKeySecret, endpoint)); + + String bucketName = readOssBucketName(); + String objectName = Paths.get(readOssBaseDir(), getObjectNameFromLogPath(logPath)).toString(); + + try { + logger.info("send remote log {} to OSS {}", logPath, objectName); + PutObjectRequest putObjectRequest = new PutObjectRequest(bucketName, objectName, new File(logPath)); + ossClient.putObject(putObjectRequest); + } catch (Exception e) { + logger.error("error while sending remote log {} to OSS {}", logPath, objectName, e); + } + } + + @Override + public void getRemoteLog(String logPath) { + String accessKeyId = readOssAccessKeyId(); + String accessKeySecret = readOssAccessKeySecret(); + String endpoint = readOssEndpoint(); + OSS ossClient = OssClientFactory.buildOssClient(new OssConnection(accessKeyId, accessKeySecret, endpoint)); + + String bucketName = readOssBucketName(); + String objectName = Paths.get(readOssBaseDir(), getObjectNameFromLogPath(logPath)).toString(); + + try { + logger.info("get remote log on OSS {} to {}", objectName, logPath); + ossClient.getObject(new GetObjectRequest(bucketName, objectName), new File(logPath)); + } catch (Exception e) { + logger.error("error while getting remote log on OSS {} to {}", objectName, logPath); + } + } + + private String getObjectNameFromLogPath(String logPath) { + Path path = Paths.get(logPath); + int nameCount = path.getNameCount(); + if (nameCount < 2) { Review Comment: can we use constants for magic number `2`? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
