Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Apr 1, 2024
Merged

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

merged 1 commit into from
Apr 1, 2024

Conversation

pegasas
Copy link
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
Member

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

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

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
Member

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

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

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

@sonarcloud SonarCloud
Copy link

sonarcloud 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
59 of 61 checks passed
@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
3.3.0 backend document DSIP improvement make more easy to user or prompt friendly ready-to-merge
Projects
None yet
Milestone
3.3.0
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