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

[Improvement-17010][DataX] Change datax_channel_count to a custom parameter #17032

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

[Improvement-17010][DataX] Change datax_channel_count to a custom parameter #17032

wants to merge 7 commits into from

Conversation

BruceWong96
Copy link
Contributor

@BruceWong96 BruceWong96 commented Feb 27, 2025 •
edited by SbloodyS
Loading

Purpose of the pull request

Change datax_channel_count to a custom parameter.
close #17010

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

@BruceWong96 BruceWong96 changed the title [Improvement-17010][DataX] When using datax to synchronize data in a non-custom json template, you want to add configuration items to modify the channel, otherwise channel=1 forever [Improvement-17010][DataX] Change datax_channel_count to a custom parameter Feb 27, 2025
@BruceWong96
Copy link
Contributor Author

@SbloodyS PTAL.

@SbloodyS SbloodyS added this to the 3.3.0 milestone Feb 28, 2025
@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Feb 28, 2025
SbloodyS
SbloodyS reviewed Feb 28, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

Please run mvn spotless:apply to format code. @BruceWong96

@BruceWong96
Copy link
Contributor Author

Please run mvn spotless:apply to format code. @BruceWong96

Done.
But now it seems I've encountered some other problems, can you help me?

Thanks.

@BruceWong96 BruceWong96 requested a review from SbloodyS March 4, 2025 06:14
@ruanwenjun
Copy link
Member

Please run mvn spotless:apply to format code. @BruceWong96

Done. But now it seems I've encountered some other problems, can you help me?

Thanks.

Some ci problem can be fixed by #17041

@BruceWong96
Copy link
Contributor Author

Please retry analysis of this Pull-Request directly on SonarQube Cloud

@SbloodyS How do i retry analysis of this PR ?
Thanks.

@BruceWong96
Copy link
Contributor Author

Please run mvn spotless:apply to format code. @BruceWong96

Done. But now it seems I've encountered some other problems, can you help me?
Thanks.

Some ci problem can be fixed by #17041

OK, finished.

SbloodyS
SbloodyS approved these changes Mar 8, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

ruanwenjun
ruanwenjun reviewed Mar 10, 2025
View reviewed changes
...-task-datax/src/main/java/org/apache/dolphinscheduler/plugin/task/datax/DataxParameters.java
/**
* channel count
*/
private int channelCount = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We need to expose this into UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the following documents, the channel can be directly configured in the JSON.
ds-doc-datax
hdfswriter

What do you think about it ?

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

If so, this PR seems meanless, it doesn't change the usage.

Copy link
Contributor Author

@BruceWong96 BruceWong96 Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If so, this PR seems meanless, it doesn't change the usage.

Sorry, I might have misunderstood this issue.
After reviewing the issue, it’s clear that custom configurations are fully handled through JSON.
Modifying this parameter is only required when using DS’s default configuration.

To address this, we should add a channel configuration option in the UI to let users adjust the channel count setting.

Is my understanding correct?
Thanks.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 60%)
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@BruceWong96 BruceWong96 requested a review from ruanwenjun March 12, 2025 02:38
@sdhzwc
Copy link
Contributor

sdhzwc commented Mar 14, 2025

2025-03-14 16 50 18
@SbloodyS @ruanwenjun cc, General practice

@SbloodyS SbloodyS modified the milestones: 3.3.0-alpha, 3.3.1 Mar 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SbloodyS SbloodyS SbloodyS approved these changes

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

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

@ruanwenjun ruanwenjun Awaiting requested review from ruanwenjun

At least 2 approving reviews are required to merge this pull request.

Assignees

@BruceWong96 BruceWong96

Labels
backend improvement make more easy to user or prompt friendly test
Projects
None yet
Milestone
3.3.1
4 participants
@BruceWong96 @ruanwenjun @sdhzwc @SbloodyS