Skip to content

[python] Add checker pd attr param and fix switch example#7818

Merged
CalvinKirs merged 3 commits into
apache:devfrom
zhongjiajie:py-bug-ex-task-switch
Jan 5, 2022
Merged

[python] Add checker pd attr param and fix switch example#7818
CalvinKirs merged 3 commits into
apache:devfrom
zhongjiajie:py-bug-ex-task-switch

Conversation

@zhongjiajie
Copy link
Copy Markdown
Member

  • Correct submit self.param to java gateway
  • Fix missing parameter for switch example
  • Add mechanism checker before submit to java gateway

close: #7803

* Correct submit self.param to java gateway
* Fix missing parameter for switch example
* Add mechanism checker before submit to java gateway

close: apache#7803
@zhongjiajie zhongjiajie added bug Something isn't working enhancement New feature or request Pyscheduler labels Jan 5, 2022
@zhongjiajie zhongjiajie self-assigned this Jan 5, 2022
@zhongjiajie
Copy link
Copy Markdown
Member Author

@devosend PTAL if you have time, thanks

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #7818 (f271f5b) into dev (ab89e43) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #7818      +/-   ##
============================================
+ Coverage     40.89%   41.00%   +0.10%     
- Complexity     3671     3684      +13     
============================================
  Files           637      637              
  Lines         26783    26934     +151     
  Branches       3017     3042      +25     
============================================
+ Hits          10954    11045      +91     
- Misses        14776    14823      +47     
- Partials       1053     1066      +13     
Impacted Files Coverage Δ
...he/dolphinscheduler/common/enums/SqoopJobType.java 0.00% <0.00%> (-88.89%) ⬇️
...r/plugin/registry/zookeeper/ZookeeperRegistry.java 48.21% <0.00%> (-7.15%) ⬇️
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...inscheduler/common/task/sqoop/SqoopParameters.java 74.00% <0.00%> (-2.00%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 50.70% <0.00%> (-1.41%) ⬇️
...er/api/controller/ProcessDefinitionController.java 47.56% <0.00%> (-1.19%) ⬇️
...org/apache/dolphinscheduler/remote/utils/Host.java 40.00% <0.00%> (+2.22%) ⬆️
...api/service/impl/ProcessDefinitionServiceImpl.java 32.21% <0.00%> (+6.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab89e43...f271f5b. Read the comment docs.

@property
def param_json(self) -> Optional[List[Dict]]:
"""Return param json base on self.param."""
if self.param is None or not self.param:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't these two judgment conditions the same?

Copy link
Copy Markdown
Member Author

@zhongjiajie zhongjiajie Jan 5, 2022

Choose a reason for hiding this comment

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

It is two behavior, one is for None and other for {}(empty dict). But we can also combine to single statement not self.param too.

Copy link
Copy Markdown
Member Author

@zhongjiajie zhongjiajie Jan 5, 2022

Choose a reason for hiding this comment

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

@devosend WDYT, should we combine or separate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should combine,beause and None and {} are empty. maybe we can add a comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to add this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I add f271f5b to correct it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LGTM

Thanks for your review

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jan 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Copy Markdown
Member

@CalvinKirs CalvinKirs left a comment

Choose a reason for hiding this comment

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

LGTM

@CalvinKirs CalvinKirs merged commit b1edce2 into apache:dev Jan 5, 2022
@zhongjiajie zhongjiajie deleted the py-bug-ex-task-switch branch January 5, 2022 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request Pyscheduler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [python] Missing set global parameter in switch task example

4 participants