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.

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

[Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance. #12506

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

BongBongBang
Copy link
Contributor

@BongBongBang BongBongBang commented Oct 24, 2022

Purpose of the pull request

Brief change log

[Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.

Verify this pull request

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

@caishunfeng caishunfeng added the first time contributor First-time contributor label Oct 25, 2022
@caishunfeng caishunfeng added this to the 3.2.0 milestone Oct 25, 2022
@BongBongBang BongBongBang requested review from caishunfeng and removed request for SbloodyS and ruanwenjun October 27, 2022 10:48
caishunfeng
caishunfeng previously approved these changes Oct 27, 2022
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM if CI pass.

@BongBongBang
Copy link
Contributor Author

BongBongBang commented Oct 29, 2022

Oops. I run ./mvnw spotless:check locally ,and there doesn't have violations...And after run ./.mvnw spotless:apply ,there has nothing changed.....How could i fix this...Any idea?

@BongBongBang BongBongBang removed their assignment Oct 29, 2022
@BongBongBang
Copy link
Contributor Author

。。。

@caishunfeng
Copy link
Contributor

Oops. I run ./mvnw spotless:check locally ,and there doesn't have violations...And after run ./.mvnw spotless:apply ,there has nothing changed.....How could i fix this...Any idea?

I approve to run the CI (Due to you are the first contributor) and will check it after the CI finish.

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2022

Codecov Report

Merging #12506 (5758f8a) into dev (dcb8070) will decrease coverage by 0.02%.
The diff coverage is 31.57%.

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

@@             Coverage Diff              @@
##                dev   #12506      +/-   ##
============================================
- Coverage     39.05%   39.03%   -0.03%     
+ Complexity     4186     4183       -3     
============================================
  Files          1043     1043              
  Lines         39506    39490      -16     
  Branches       4544     4526      -18     
============================================
- Hits          15430    15413      -17     
  Misses        22319    22319              
- Partials       1757     1758       +1     
Impacted Files Coverage Δ
.../dao/repository/impl/ProcessDefinitionDaoImpl.java 4.34% <0.00%> (-5.66%) ⬇️
...r/server/master/service/MasterFailoverService.java 53.28% <100.00%> (+1.04%) ⬆️
...erver/master/processor/queue/TaskEventService.java 69.64% <0.00%> (-10.72%) ⬇️
...dolphinscheduler/remote/future/ResponseFuture.java 81.96% <0.00%> (-1.64%) ⬇️
...hinscheduler/plugin/alert/script/ScriptSender.java 70.58% <0.00%> (-1.64%) ⬇️
...cheduler/plugin/alert/dingtalk/DingTalkSender.java 34.13% <0.00%> (-0.78%) ⬇️
...pache/dolphinscheduler/common/utils/DateUtils.java 68.30% <0.00%> (-0.55%) ⬇️
...rver/master/runner/task/BlockingTaskProcessor.java 75.86% <0.00%> (-0.55%) ⬇️
...inscheduler/plugin/registry/etcd/EtcdRegistry.java 50.69% <0.00%> (-0.35%) ⬇️
.../dolphinscheduler/plugin/task/datax/DataxTask.java 36.62% <0.00%> (-0.26%) ⬇️
... and 19 more

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

@BongBongBang
Copy link
Contributor Author

BongBongBang commented Oct 29, 2022

SonarCloud reported that MasterFailoverService constructor has 8 parameters which is more than 7.....I was blocked by the TaskInstanceDao that it's added after mine

@BongBongBang
Copy link
Contributor Author

BongBongBang commented Oct 29, 2022

Let me remove ProcessDefinitionDao parameter from constructor and inject it

@caishunfeng
Copy link
Contributor

I had approved to run CI (due to first time contributor )

@caishunfeng
Copy link
Contributor

@BongBongBang
Copy link
Contributor Author

@BongBongBang please use spotless to format code style. see code style https://dolphinscheduler.apache.org/en-us/docs/dev/user_doc/contribute/development-environment-setup.html

Actually, I do did that maven command before submission because of the failure about spotless before...... Maybe i forgot again....My fault.....

@BongBongBang
Copy link
Contributor Author

Like chinese idiom, 好事多磨

@caishunfeng
Copy link
Contributor

@BongBongBang please use spotless to format code style. see code style https://dolphinscheduler.apache.org/en-us/docs/dev/user_doc/contribute/development-environment-setup.html

Actually, I do did that maven command before submission because of the failure about spotless before...... Maybe i forgot again....My fault.....

You can set the pre-commit (see the doc above), and then it will auto spotless every commit.

Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM if CI pass

@sonarcloud
Copy link

sonarcloud bot commented Nov 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

28.6% 28.6% Coverage
0.0% 0.0% Duplication

@caishunfeng caishunfeng merged commit 7cdb926 into apache:dev Nov 3, 2022
@caishunfeng
Copy link
Contributor

Hi @BongBongBang , thanks for your contribution and welcome to join community 🎉 . Looking forward to your next contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend first time contributor First-time contributor improvement make more easy to user or prompt friendly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants