[Feature-13331][Remote Logging] Add support for writing task logs to OSS by rickchengx · Pull Request #13332 · apache/dolphinscheduler

rickchengx · 2023-01-04T06:43:06Z

Purpose of the pull request

close: [Feature][Remote Logging] Add support for writing task logs to OSS #13331
A sub-task of [DSIP-18][Remote Logging] Add support for writing task logs to remote storage #13017

Add support for writing task logs to OSS

Brief change log

Task log writing

master / worker will send the task log to the remote storage asynchronously after the task is finished if remote.logging.enable=true (By default it's false)

Task log reading

master / worker will first read the task log if the log file exists on the local file system
if the task log file does not exist, master / worker will download the task log file from the remote storage to the local file system and then read the task log file

Log retention

Log retention can be directly configured using the retention policy provided by the remote storage
E.g., log retention on OSS

Verify this pull request

manually tested

Configure related variables in common.properties

# Whether to enable remote logging
remote.logging.enable=true
# if remote.logging.enable = true, set the target of remote logging
remote.logging.target=OSS
# log base directory
remote.logging.base.dir=logs
# 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>

Create a shell task
View the task log

Check that the task log is sent to OSS

delete the local task log file
View the task log again. The task log will be download from OSS.

codecov-commenter · 2023-01-04T07:01:33Z

Codecov Report

Merging #13332 (02cd619) into dev (a6550bc) will decrease coverage by 0.11%.
The diff coverage is 6.89%.

@@             Coverage Diff              @@
##                dev   #13332      +/-   ##
============================================
- Coverage     39.57%   39.47%   -0.11%     
- Complexity     4373     4377       +4     
============================================
  Files          1097     1102       +5     
  Lines         41293    41437     +144     
  Branches       4723     4736      +13     
============================================
+ Hits          16342    16356      +14     
- Misses        23138    23263     +125     
- Partials       1813     1818       +5

Impacted Files	Coverage Δ
...e/dolphinscheduler/common/constants/Constants.java	`75.00% <ø> (ø)`
...r/common/log/remote/RemoteLogHandleThreadPool.java	`0.00% <0.00%> (ø)`
...ler/common/log/remote/RemoteLogHandlerFactory.java	`0.00% <0.00%> (ø)`
...nscheduler/common/log/remote/RemoteLogService.java	`0.00% <0.00%> (ø)`
...hinscheduler/common/log/remote/RemoteLogUtils.java	`0.00% <0.00%> (ø)`
.../server/master/runner/WorkflowExecuteRunnable.java	`10.09% <0.00%> (-0.06%)`	⬇️
...duler/remote/processor/LoggerRequestProcessor.java	`0.00% <0.00%> (ø)`
...duler/plugin/task/api/AbstractCommandExecutor.java	`0.00% <0.00%> (ø)`
...lphinscheduler/plugin/task/api/utils/LogUtils.java	`26.96% <ø> (ø)`
...heduler/common/log/remote/OssRemoteLogHandler.java	`16.00% <16.00%> (ø)`
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zhongjiajie · 2023-01-04T13:40:42Z

dolphinscheduler-common/src/main/resources/common.properties

+
+# remote logging
+remote.logging.enable=false
+# if remote.logging.enable = true, set the target of remote logging


should we add a comment to tell users we support oss only currently?

Sure, I'll add it in the doc.

zhongjiajie · 2023-01-04T13:43:05Z

dolphinscheduler-common/src/main/resources/common.properties

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


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

I also thought about this problem, and I rechecked the relevant configuration in Airflow.

I think we could use remote.logging.base.dir instead, which can be shared by multiple plugins (OSS / S3 / GCS)

zhongjiajie · 2023-01-04T13:44:24Z

...ler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandler.java

+
+    void sendRemoteLog(String logPath);
+
+    void getRemoteLog(String logPath);


should we also not add an interface to test whether the current remote target work or not?

Hi, @zhongjiajie , I am not sure about this.

Do you mean that the user can click a button somewhere on the front end to test whether the remote storage is available?

The usage scenario of remote logging is somewhat similar to that of the resource center, but it seems that StorageOperate does not currently have a similar interface.

But I add checkBucketNameExists(), like OssStorageOperator and S3StorageOperator.

zhongjiajie · 2023-01-04T13:48:45Z

...mon/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java

+
+    public RemoteLogHandler getRemoteLogHandler() {
+        if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) {
+            if ("OSS".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET))) {


we have getUpperCaseString method now

Suggested change

if ("OSS".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET))) {

if ("OSS".equals(PropertyUtils.getUpperCaseString(Constants.REMOTE_LOGGING_TARGET))) {

Sure, I'll use getUpperCaseString.

zhongjiajie · 2023-01-04T13:49:08Z

...mon/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java

+public class RemoteLogHandlerFactory {
+
+    public RemoteLogHandler getRemoteLogHandler() {
+        if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) {


how about use PropertyUtils.getBoolean directly?

zhongjiajie · 2023-01-04T13:52:49Z

...-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/OssRemoteLogHandler.java

+    private String getObjectNameFromLogPath(String logPath) {
+        Path path = Paths.get(logPath);
+        int nameCount = path.getNameCount();
+        if (nameCount < 2) {


can we use constants for magic number 2?

zhongjiajie · 2023-01-04T13:54:18Z

.../src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java

+        if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) {
+            if (taskInstance.getHost().endsWith(masterAddress.split(":")[1])) {


use PropertyUtils.getBoolean instead of string, and we have COLON in constants for :

zhongjiajie · 2023-01-04T13:57:40Z

.../src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java

+                    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);


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

Sure, I'll look into it.

zhongjiajie · 2023-01-04T14:00:21Z

...rc/main/java/org/apache/dolphinscheduler/server/worker/runner/WorkerTaskExecuteRunnable.java

+                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);


why we have to sleep for 5s here? can we run continue when we get log path immediately?

The reason for waiting here is to allow the final output of the log to be written to the local file
Here is an example.

The green box in the figure is the last output of the task log, which is later than the time when the log is uploaded to the remote storage.

Therefore, if the thread does not use sleep(), the log uploaded to the remote storage will lose the statements in the green box.

WDYT, or is there any other better way to flush the final output of the log?

If you trigger sendRemoteLogIfNeeded in afterExecute, it does have to sleep for a few seconds, because when handling large amount of logs, async flush will take a lot of time.

So, I recommend you trigger in logHandle when handling FINALIZE_SESSION_MARKER which marks that log appender will be closed in few seconds, flush logic in ds(you can check in parseProcessOutput) makes sure that no logs will reach in these few seconds and we don't have to wait for five seconds any more.

If you trigger sendRemoteLogIfNeeded in afterExecute, it does have to sleep for a few seconds, because when handling large amount of logs, async flush will take a lot of time.

So, I recommend you trigger in logHandle when handling FINALIZE_SESSION_MARKER which marks that log appender will be closed in few seconds, flush logic in ds(you can check in parseProcessOutput) makes sure that no logs will reach in these few seconds and we don't have to wait for five seconds any more.

Hi, @Radeity Thanks a lot for your suggestion! I think this is a good idea and I'll look into it.

rickchengx · 2023-01-05T02:32:32Z

Hi, @zhongjiajie , thanks a lot for your review and comments.

I will carefully modify according to the suggestions and add related UT and documents.

rickchengx · 2023-01-17T07:23:27Z

Hi, @zhongjiajie @Radeity, thanks again for your kind review. I have modified this PR according to your comments and suggestions.

Here are some brief changes:

Remove the Thread.sleep() in WorkerTaskExecuteRunnable

send the task log to the remote storage in clear() method in AbstractCommandExecutor, so there is no need to sleep anymore.

Note that not all tasks will use logHandle and FINALIZE_SESSION_MARKER, such as ZEPPELIN task and the tasks executed on master. So this PR also sends the task log to the remote storage in afterExecuted() and afterThrowing() on worker (if the task does not use logHandle) and taskFinished on master (if the task is executed on master).

Here is some examples:

Shell Task (use logHandle)

Zeppelin Task (does not use logHandle)

Add the doc

I am not sure if the location of this doc is appropriate.

rickchengx · 2023-01-18T06:21:35Z

In order to prevent remote logging from affecting the end processing process of the task, the log is written to the remote storage in an asynchronous manner.

rickchengx · 2023-02-07T09:34:49Z

Hi, @zhongjiajie , could you please help review this again?

...mon/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogHandlerFactory.java

...ler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogService.java

...duler-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/RemoteLogUtils.java

...emote/src/main/java/org/apache/dolphinscheduler/remote/processor/LoggerRequestProcessor.java

caishunfeng · 2023-02-09T12:50:16Z

...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractCommandExecutor.java

        this.logBuffer = new LinkedBlockingQueue<>();
+
+        if (this.taskRequest != null) {
+            this.taskRequest.setLogHandleEnable(true);


Why set default true? Please add some comments.

I've changed logHandleEnable to logBufferEnable for better readability.

This PR records whether the logBuffer is enabled in the task context for the following resaons:

There are two types of tasks:

Tasks use logBuffer to cache logs (tasks that use ShellCommandExecutor which extends AbstractCommandExecutor)

At the end of this type of task, the log is not completely written to the file system, so it needs to write the log to the remote storage when the cache is finally emptied

it sends the task log to the remote storage in clear() method in AbstractCommandExecutor as below

Tasks that not use logBuffer

Note that not all tasks will use logBuffer , such as ZEPPELIN task and the tasks executed on master. So this PR also sends the task log to the remote storage in afterExecuted() and afterThrowing() on worker (if the task does not use logBuffer) and taskFinished on master (if the task is executed on master).

rickchengx · 2023-02-10T03:11:15Z

Hi, @caishunfeng , thanks a lot for your review and comments. I've modified it according to your suggestions, could you please help review this again?

caishunfeng

LGTM

caishunfeng · 2023-02-12T06:35:04Z

Hi, @caishunfeng , thanks a lot for your review and comments. I've modified it according to your suggestions, could you please help review this again?

Sure, and please resolve the conflicts.

rickchengx · 2023-02-13T02:36:33Z

Hi, @caishunfeng , I've resolved the conflicts.

zhongjiajie

LGTM overall, one more things, we should add our new adding docs remote-logging.md to https://github.com/apache/dolphinscheduler/blob/dev/docs/configs/docsdev.js file, otherwise your new docs will not show in our website

rickchengx · 2023-02-13T04:11:49Z

https://github.com/apache/dolphinscheduler/blob/dev/docs/configs/docsdev.js

Sure, I'll look into it.

sonarqubecloud · 2023-02-13T04:52:49Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

8.4% Coverage
0.0% Duplication

rickchengx · 2023-02-13T05:09:47Z

Hi, @zhongjiajie , I've add configs in docsdev.js

zhongjiajie

LGTM now

rickchengx requested review from SbloodyS, caishunfeng, ruanwenjun and zhuangchong as code owners January 4, 2023 06:43

github-actions bot added the backend label Jan 4, 2023

rickchengx force-pushed the Feature-13331 branch from 952fff4 to d06cc4a Compare January 4, 2023 13:16

zhongjiajie reviewed Jan 4, 2023

View reviewed changes

EricGao888 assigned rickchengx Jan 5, 2023

EricGao888 added this to the 3.2.0 milestone Jan 5, 2023

EricGao888 added feature new feature improvement make more easy to user or prompt friendly 3.2.0 labels Jan 5, 2023

rickchengx force-pushed the Feature-13331 branch from d06cc4a to dfe6b71 Compare January 17, 2023 06:45

rickchengx requested review from EricGao888 and Tianqi-Dotes as code owners January 17, 2023 06:45

github-actions bot added the document label Jan 17, 2023

rickchengx force-pushed the Feature-13331 branch from dfe6b71 to 46d77ec Compare January 17, 2023 07:02

rickchengx force-pushed the Feature-13331 branch 2 times, most recently from 9700d2b to c7b29cf Compare January 17, 2023 07:44

rickchengx marked this pull request as draft January 17, 2023 08:48

rickchengx force-pushed the Feature-13331 branch from c7b29cf to d9f4db3 Compare January 17, 2023 08:54

rickchengx marked this pull request as ready for review January 17, 2023 08:54

rickchengx marked this pull request as draft January 18, 2023 02:24

rickchengx force-pushed the Feature-13331 branch from d9f4db3 to 4b7ba61 Compare January 18, 2023 05:59

rickchengx marked this pull request as ready for review January 18, 2023 05:59

rickchengx mentioned this pull request Jan 19, 2023

[DSIP-18][Remote Logging] Add support for writing task logs to remote storage #13017

Closed

3 tasks

rickchengx force-pushed the Feature-13331 branch 2 times, most recently from 5af18c6 to 475906d Compare February 9, 2023 08:27

caishunfeng reviewed Feb 9, 2023

View reviewed changes

rickchengx force-pushed the Feature-13331 branch 2 times, most recently from b93a2e4 to 9252c8e Compare February 10, 2023 02:32

rickchengx requested review from caishunfeng and removed request for EricGao888, SbloodyS, Tianqi-Dotes, ruanwenjun and zhuangchong February 10, 2023 03:08

rickchengx force-pushed the Feature-13331 branch 2 times, most recently from b70bed3 to 72c3daf Compare February 10, 2023 10:29

caishunfeng previously approved these changes Feb 12, 2023

View reviewed changes

caishunfeng requested a review from zhongjiajie February 12, 2023 06:31

rickchengx added 2 commits February 13, 2023 09:57

[Feature-13331][Remote Logging] Add support for writing task logs to OSS

b2fb46d

update

4ae1af8

rickchengx dismissed caishunfeng’s stale review via 4ae1af8 February 13, 2023 01:57

rickchengx force-pushed the Feature-13331 branch from 72c3daf to 4ae1af8 Compare February 13, 2023 01:57

zhongjiajie reviewed Feb 13, 2023

View reviewed changes

update docsdev.js

02cd619

zhongjiajie approved these changes Feb 13, 2023

View reviewed changes

zhongjiajie merged commit 2bd65fb into apache:dev Feb 13, 2023


		void sendRemoteLog(String logPath);

		void getRemoteLog(String logPath);

	if ("OSS".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_TARGET))) {
	if ("OSS".equals(PropertyUtils.getUpperCaseString(Constants.REMOTE_LOGGING_TARGET))) {

		if ("true".equalsIgnoreCase(PropertyUtils.getString(Constants.REMOTE_LOGGING_ENABLE))) {
		if (taskInstance.getHost().endsWith(masterAddress.split(":")[1])) {

Conversation

rickchengx commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Task log writing

Task log reading

Log retention

Verify this pull request

Uh oh!

codecov-commenter commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickchengx Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickchengx Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickchengx Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickchengx commented Jan 5, 2023

Uh oh!

rickchengx commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickchengx commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickchengx commented Feb 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickchengx commented Feb 10, 2023

rickchengx commented Jan 4, 2023 •

edited

Loading

codecov-commenter commented Jan 4, 2023 •

edited

Loading

rickchengx Jan 5, 2023 •

edited

Loading

rickchengx Jan 5, 2023 •

edited

Loading

rickchengx Jan 5, 2023 •

edited

Loading

rickchengx commented Jan 17, 2023 •

edited

Loading

rickchengx commented Jan 18, 2023 •

edited

Loading