Skip to content

[DSIP-24][RemoteLogging] Support AbsRemoteLogHandler#15769

Merged
ruanwenjun merged 1 commit intoapache:devfrom
pegasas:issues/15765
Apr 1, 2024
Merged

[DSIP-24][RemoteLogging] Support AbsRemoteLogHandler#15769
ruanwenjun merged 1 commit intoapache:devfrom
pegasas:issues/15765

Conversation

@pegasas
Copy link
Copy Markdown
Contributor

@pegasas pegasas commented Mar 26, 2024 •
edited by ruanwenjun
Loading

Purpose of the pull request

close: #15765
add azure blob remote log handler

Brief change log

add azure blob remote log handler

Verify this pull request

image

@pegasas pegasas force-pushed the issues/15765 branch 2 times, most recently from 35b160c to 50e0ccb Compare March 26, 2024 14:15
@pegasas pegasas changed the title [Improvement][RemoteLogging] Add AbsRemoteLogHandler [Improvement][RemoteLogging] Support AbsRemoteLogHandler Mar 26, 2024
@pegasas pegasas marked this pull request as ready for review March 26, 2024 14:49
@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly 3.3.0 labels Mar 27, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Mar 27, 2024
@ruanwenjun
Copy link
Copy Markdown
Member

ruanwenjun commented Mar 27, 2024

Can we move the properties related to the remote log to a separate config file, e.g. remote-log.yaml, right now, the common.properties is too large, we shouldn't add addition config into this file and the file type should be deprecated, we have a better file type like yaml.

@pegasas pegasas mentioned this pull request Mar 27, 2024
Closed
3 tasks
@pegasas
Copy link
Copy Markdown
Contributor Author

pegasas commented Mar 27, 2024

Can we move the properties related to the remote log to a separate config file, e.g. remote-log.yaml, right now, the common.properties is too large, we shouldn't add addition config into this file and the file type should be deprecated, we have a better file type like yaml.

I agree with that
create an following issue for tracking this #15774

ruanwenjun
ruanwenjun requested changes Mar 28, 2024
View reviewed changes
...-common/src/main/java/org/apache/dolphinscheduler/common/log/remote/AbsRemoteLogHandler.java Outdated
Comment on lines +63 to +68
public void init() {
accountName = readAccountName();
accountKey = readAccountKey();
containerName = readContainerName();
blobContainerClient = buildBlobContainerClient();
}
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into the constructor?
When we get the instance by

AbsRemoteLogHandler.getInstance();

We don't need to execute init since the instance has already been initialized.

Copy link
Copy Markdown
Contributor Author

@pegasas pegasas Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this is a solution.
Currently it follows the patterns with GcsRemoteLogHandler , s3remotelogger and etc.
I will create another PR for fixing this.

Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to follow the bad patterns, and the init method is not defined in the interface. You can submit another PR to fix other handler, but this handler shouldn't use init.

Copy link
Copy Markdown
Contributor Author

@pegasas pegasas Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I create an PR for fixing this #15780

@ruanwenjun ruanwenjun changed the title [Improvement][RemoteLogging] Support AbsRemoteLogHandler [DSIP][RemoteLogging] Support AbsRemoteLogHandler Mar 28, 2024
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.41667% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 39.21%. Comparing base (d39bdcb) to head (c64ea49).

❗ Current head c64ea49 differs from pull request most recent head a9e8d3f. Consider uploading reports for the commit a9e8d3f to get more accurate results

Files Patch % Lines
...heduler/common/log/remote/AbsRemoteLogHandler.java 63.04% 16 Missing and 1 partial ⚠️
...ler/common/log/remote/RemoteLogHandlerFactory.java 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15769      +/-   ##
============================================
+ Coverage     39.16%   39.21%   +0.04%     
- Complexity     4867     4880      +13     
============================================
  Files          1316     1317       +1     
  Lines         44978    45026      +48     
  Branches       4803     4806       +3     
============================================
+ Hits          17617    17656      +39     
- Misses        25477    25484       +7     
- Partials       1884     1886       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun changed the title [DSIP][RemoteLogging] Support AbsRemoteLogHandler [DSIP-24][RemoteLogging] Support AbsRemoteLogHandler Mar 28, 2024
rickchengx
rickchengx reviewed Mar 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with moving the properties related to the remote log to a separate config file.
Overall LGTM, please add related UT

@pegasas pegasas mentioned this pull request Mar 29, 2024
Merged
@pegasas pegasas force-pushed the issues/15765 branch 2 times, most recently from c8183fa to 8c01e2c Compare March 29, 2024 11:14
@pegasas
Copy link
Copy Markdown
Contributor Author

pegasas commented Mar 29, 2024 •
edited
Loading

Agree with moving the properties related to the remote log to a separate config file. Overall LGTM, please add related UT

Thanks @rickchengx .
Update UT.
image

ruanwenjun
ruanwenjun approved these changes Mar 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your PR, after the CI pass I will merge into dev.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 31, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

@ruanwenjun ruanwenjun merged commit ac0189a into apache:dev Apr 1, 2024
@pegasas pegasas deleted the issues/15765 branch April 1, 2024 02:03
@SbloodyS SbloodyS added the DSIP label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rickchengx rickchengx rickchengx left review comments

@ruanwenjun ruanwenjun ruanwenjun approved these changes

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

@caishunfeng caishunfeng Awaiting requested review from caishunfeng

Assignees

@pegasas pegasas

Labels

backend document DSIP improvement make more easy to user or prompt friendly ready-to-merge

Projects

None yet

Milestone

3.3.0-alpha

Development

Successfully merging this pull request may close these issues.

[DSIP-24][RemoteLogging] Add Azure Blob Storage RemoteLogHandler

5 participants

@pegasas @ruanwenjun @codecov-commenter @rickchengx @SbloodyS