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

[Fix-16211] Remove ExecutePath in buildJarCommand to fix Java Task in Jar Mode #16238

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

lanxing2
Copy link
Contributor

Pull Request Branch

Issue-16211

Pull Request Content

Purpose of the pull request

Fix issue 16211
The comment says "duplicated with #15902"
And should be fixed by pr 15906
But I have tested and I think PR 15906 only fix the JavaTask Java Mode, Jar Mode still have bug
ResourceAbsolutePathInLocal already have the ExecutePath, need remove it in buildJarCommand() method
Also update the UnitTest
Please check my reply in issue 16211
I deployed the latest code in apache/dolphinscheduler dev branch in my own cluster and run JavaTask in Jar Mode and it failed
After deployed my code in my own cluster, run JavaTask in Jar Mode and it succeed

Brief change log

[Fix issue 16211] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode

Verify this pull request

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

  • Updated JavaTaskTest to verify the change.
  • *Build dolphinscheduler-task-java and deployed to my own cluster and tested end to end, please check the comment in issue 16211 about the log before and after this pull request *
  • Run mvn spotless:apply and check the code format

(or)

Pull Request Code Style

Already run the mvn spotless:apply and checked no problem with the code style

Pull Request Notice

Pull Request Notice

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

lanxing2 added 2 commits June 28, 2024 21:03
Update JavaTask.java Fix buildJarCommand()
ExecutePath already exist in ResourceAbsolutePath, no need to add it again
ResourceAbsolutePathInLocal contains the executePath, need update the Unit test accordingly. Also need this update for JavaTask buildJarCommand Change
@lanxing2
Copy link
Contributor Author

Changed the branch name and requested a new PR

@ruanwenjun ruanwenjun changed the title [Fix 16211][java-task] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode [Fix-16211][java-task] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode Jun 28, 2024
@ruanwenjun ruanwenjun added this to the 3.2.2 milestone Jun 28, 2024
@ruanwenjun
Copy link
Member

Good catch, sorry for missing this, we can use directly abosolute path is enough.

@SbloodyS SbloodyS added the bug Something isn't working label Jun 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.17%. Comparing base (225e969) to head (37dfe5d).

Current head 37dfe5d differs from pull request most recent head d37df3c

Please upload reports for the commit d37df3c to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #16238      +/-   ##
============================================
- Coverage     41.18%   41.17%   -0.02%     
+ Complexity     5097     5096       -1     
============================================
  Files          1391     1391              
  Lines         43838    43837       -1     
  Branches       4652     4652              
============================================
- Hits          18056    18050       -6     
- Misses        24013    24018       +5     
  Partials       1769     1769              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@SbloodyS SbloodyS changed the title [Fix-16211][java-task] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode [Fix-16211] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode Jun 28, 2024
@SbloodyS SbloodyS changed the title [Fix-16211] Remove ExecutePath in buildJarCommand() to fix Java Task in Jar Mode [Fix-16211] Remove ExecutePath in buildJarCommand to fix Java Task in Jar Mode Jun 28, 2024
@SbloodyS SbloodyS changed the title [Fix-16211] Remove ExecutePath in buildJarCommand to fix Java Task in Jar Mode [Fix-16211] Remove ExecutePath in buildJarCommand to fix Java Task in Jar Mode Jun 28, 2024
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

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.

+1

@ruanwenjun ruanwenjun merged commit d42ac96 into apache:dev Jun 29, 2024
64 of 67 checks passed
Copy link

boring-cyborg bot commented Jun 29, 2024

Awesome work, congrats on your first merged pull request!

ruanwenjun pushed a commit to ruanwenjun/dolphinscheduler that referenced this pull request Jun 29, 2024
ruanwenjun added a commit that referenced this pull request Jun 29, 2024
… Jar Mode (#16238) (#16241)

(cherry picked from commit d42ac96)

Co-authored-by: blink <lanxing2@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants