[Fix-13828][api] Fix the problem of work groups have two defult records #13829

ralphgj · 2023-03-30T08:24:27Z

Purpose of the pull request

close #13828

Brief change log

Fix the problem of work groups have two defult records

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

SbloodyS · 2023-03-30T08:29:52Z

cc @ruanwenjun @caishunfeng

codecov-commenter · 2023-03-30T08:51:02Z

Codecov Report

Merging #13829 (aac9a02) into dev (1e8d09e) will decrease coverage by 0.02%.
The diff coverage is 22.97%.

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

@@             Coverage Diff              @@
##                dev   #13829      +/-   ##
============================================
- Coverage     39.10%   39.08%   -0.02%     
+ Complexity     4440     4434       -6     
============================================
  Files          1132     1142      +10     
  Lines         42163    42014     -149     
  Branches       4746     4740       -6     
============================================
- Hits          16487    16423      -64     
+ Misses        23868    23790      -78     
+ Partials       1808     1801       -7

Impacted Files	Coverage Δ
.../dolphinscheduler/alert/AlertRequestProcessor.java	`0.00% <0.00%> (-93.75%)`	⬇️
...org/apache/dolphinscheduler/alert/AlertServer.java	`0.00% <0.00%> (-45.95%)`	⬇️
.../dolphinscheduler/api/aspect/CacheEvictAspect.java	`5.12% <0.00%> (ø)`
...low/instance/pause/pause/PauseExecuteFunction.java	`0.00% <0.00%> (ø)`
...or/workflow/instance/stop/StopExecuteFunction.java	`0.00% <0.00%> (ø)`
.../apache/dolphinscheduler/api/rpc/ApiRpcClient.java	`60.00% <0.00%> (ø)`
...er/api/service/impl/MetricsCleanUpServiceImpl.java	`14.28% <0.00%> (ø)`
...uler/api/service/impl/TaskInstanceServiceImpl.java	`51.79% <0.00%> (ø)`
...hinscheduler/common/constants/TenantConstants.java	`0.00% <0.00%> (ø)`
...apache/dolphinscheduler/common/utils/LogUtils.java	`0.00% <0.00%> (ø)`
... and 131 more

... and 3 files with indirect coverage changes

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

rickchengx · 2023-03-30T09:52:20Z

...r-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java

@@ -293,7 +293,9 @@ private List<WorkerGroup> getWorkerGroups(List<Integer> ids) {
            workerGroups = workerGroupMapper.queryAllWorkerGroup();
        }
        Optional<Boolean> containDefaultWorkerGroups = workerGroups.stream()
-                .map(workerGroup -> Constants.DEFAULT_WORKER_GROUP.equals(workerGroup.getName())).findAny();
+                .map(workerGroup -> Constants.DEFAULT_WORKER_GROUP.equals(workerGroup.getName()))
+                .filter(result -> result)


Hi, @ralphgj , could you please explain more about this filter()?

The containDefaultWorkerGroups method should verify whether it contains the default work group. The map method only converts the work group into true/false, which indicates whether it is the default work group or not. The findAny method will select the first one. If the first work group converted by map is not the default work group, the result of containDefaultWorkerGroups will be false, but the actual work group retrieved contains the default work group.

Radeity

Hi, @ralphgj , my suggestion is to find out why is it possible to create another worker group named default and fix that bug, rather than directly filter here, WDYT?

ralphgj · 2023-03-30T10:36:46Z

Hi, @ralphgj , my suggestion is to find out why is it possible to create another worker group named default and fix that bug, rather than directly filter here, WDYT?

@Radeity Currently, the default worker group contains all existing worker servers. However, in some cases, I do not want the default worker group include certain special worker servers.

Radeity · 2023-03-30T11:02:40Z

Hi, @ralphgj , thanks for your explanation for the use of filter above, i can get you point now, another humble suggestion, maybe use anyMatch(result -> result) is better : )

ralphgj · 2023-03-30T11:22:36Z

@Radeity thanks for your suggestion. I have resubmitted it. Could you please review it again?

Radeity · 2023-03-30T11:24:39Z

@Radeity thanks for your suggestion. I have resubmitted it. Could you please review it again?

LGTM 👍

sonarcloud · 2023-03-31T02:15:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

75.0% Coverage
0.0% Duplication

SbloodyS · 2023-03-31T02:21:57Z

Hi, @ralphgj , my suggestion is to find out why is it possible to create another worker group named default and fix that bug, rather than directly filter here, WDYT?

@Radeity Currently, the default worker group contains all existing worker servers. However, in some cases, I do not want the default worker group include certain special worker servers.

In this case, I think we should do a validation logic to avoid creating worker groups with the same name. Removing duplicates may lead to confusion among different users regarding worker groups with the same name.

Radeity · 2023-03-31T02:43:19Z

In this case, I think we should do a validation logic to avoid creating worker groups with the same name. Removing duplicates may lead to confusion among different users regarding worker groups with the same name.

Hi, @SbloodyS , i think this PR has a correct fix, but wrong issue description which may be confused. Actually, we have this validation, user can not create worker groups with the same name, this bug happens because of using findAny(). @ralphgj , can you update your description to make it clearer?

SbloodyS · 2023-03-31T02:53:47Z

Hi, @SbloodyS , i think this PR has a correct fix, but wrong issue description which may be confused. Actually, we have this validation, user can not create worker groups with the same name, this bug happens because of using findAny(). @ralphgj , can you update your description to make it clearer?

Yes. You are right. I didn't look carefully.

SbloodyS

+1

ralphgj · 2023-03-31T03:14:01Z

@Radeity Sure, I have updated it.

…defult records #13829

ralphgj requested review from caishunfeng and SbloodyS as code owners March 30, 2023 08:24

github-actions bot added the backend label Mar 30, 2023

SbloodyS assigned ralphgj Mar 30, 2023

SbloodyS added bug Something isn't working first time contributor First-time contributor 3.1.x for 3.1.x version labels Mar 30, 2023

SbloodyS added this to the 3.1.5 milestone Mar 30, 2023

rickchengx reviewed Mar 30, 2023

View reviewed changes

Radeity requested changes Mar 30, 2023

View reviewed changes

[Fix-13828][api] Fix the problem of work groups have two defult records

84ba2b3

ralphgj force-pushed the bugfix-13828 branch from 9ada51f to 84ba2b3 Compare March 30, 2023 11:18

SbloodyS approved these changes Mar 31, 2023

View reviewed changes

SbloodyS merged commit 8f3a23d into apache:dev Mar 31, 2023

zhuangchong modified the milestones: 3.1.5, 3.1.6 Apr 6, 2023

zhuangchong pushed a commit that referenced this pull request Apr 13, 2023

cherry-pick [Fix-13828][api] Fix the problem of work groups have two …

7e134b2

…defult records #13829

zhuangchong added the release cherry-pick 3.1.x label Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix-13828][api] Fix the problem of work groups have two defult records #13829

[Fix-13828][api] Fix the problem of work groups have two defult records #13829

ralphgj commented Mar 30, 2023

SbloodyS commented Mar 30, 2023

codecov-commenter commented Mar 30, 2023 •

edited

Loading

rickchengx Mar 30, 2023

ralphgj Mar 30, 2023 •

edited

Loading

Radeity left a comment •

edited

Loading

ralphgj commented Mar 30, 2023

Radeity commented Mar 30, 2023

ralphgj commented Mar 30, 2023

Radeity commented Mar 30, 2023

sonarcloud bot commented Mar 31, 2023

SbloodyS commented Mar 31, 2023 •

edited

Loading

Radeity commented Mar 31, 2023 •

edited

Loading

SbloodyS commented Mar 31, 2023

SbloodyS left a comment

ralphgj commented Mar 31, 2023

[Fix-13828][api] Fix the problem of work groups have two defult records #13829

[Fix-13828][api] Fix the problem of work groups have two defult records #13829

Conversation

ralphgj commented Mar 30, 2023

Purpose of the pull request

Brief change log

Verify this pull request

SbloodyS commented Mar 30, 2023

codecov-commenter commented Mar 30, 2023 • edited Loading

Codecov Report

rickchengx Mar 30, 2023

Choose a reason for hiding this comment

ralphgj Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Radeity left a comment • edited Loading

Choose a reason for hiding this comment

ralphgj commented Mar 30, 2023

Radeity commented Mar 30, 2023

ralphgj commented Mar 30, 2023

Radeity commented Mar 30, 2023

sonarcloud bot commented Mar 31, 2023

SbloodyS commented Mar 31, 2023 • edited Loading

Radeity commented Mar 31, 2023 • edited Loading

SbloodyS commented Mar 31, 2023

SbloodyS left a comment

Choose a reason for hiding this comment

ralphgj commented Mar 31, 2023

codecov-commenter commented Mar 30, 2023 •

edited

Loading

ralphgj Mar 30, 2023 •

edited

Loading

Radeity left a comment •

edited

Loading

SbloodyS commented Mar 31, 2023 •

edited

Loading

Radeity commented Mar 31, 2023 •

edited

Loading