Skip to content

fix: Save task seatunnel error #14129

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 5 commits into from
May 19, 2023
Merged

fix: Save task seatunnel error #14129

merged 5 commits into from
May 19, 2023

Conversation

zhongjiajie
Copy link
Member

@zhongjiajie zhongjiajie commented May 17, 2023

  • fix error when save task seatunnel
  • support all version of seatunnel startup script by using string instead of engine enum
  • Remove unnecessary code in task seatunnel

closed #14114

@github-actions github-actions bot added backend UI ui and front end related labels May 17, 2023
@zhongjiajie zhongjiajie changed the title fix: Save task seatunnel error [WIP] fix: Save task seatunnel error May 17, 2023
@zhongjiajie zhongjiajie added this to the 3.1.7 milestone May 17, 2023
@zhongjiajie zhongjiajie added the 3.1.x for 3.1.x version label May 17, 2023
@zhongjiajie zhongjiajie self-assigned this May 17, 2023
@zhongjiajie zhongjiajie marked this pull request as ready for review May 17, 2023 10:28
@zhongjiajie zhongjiajie changed the title [WIP] fix: Save task seatunnel error fix: Save task seatunnel error May 18, 2023
Comment on lines -20 to -23
public enum EngineEnum {

FLINK("${SEATUNNEL_HOME}/bin/start-seatunnel-flink.sh"),
SPARK("${SEATUNNEL_HOME}/bin/start-seatunnel-spark.sh"),
Copy link
Member Author

Choose a reason for hiding this comment

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

delete the enum class due to seatunnel changing its startup script, so currently we have to support

seatunnel.sh
start-seatunnel-flink-13-connector-v2.sh
start-seatunnel-flink-15-connector-v2.sh
start-seatunnel-flink-connector-v2.sh
start-seatunnel-flink.sh
start-seatunnel-spark-2-connector-v2.sh
start-seatunnel-spark-3-connector-v2.sh
start-seatunnel-spark-connector-v2.sh
start-seatunnel-spark.sh

nine startups in both frontend and backend. Since we do not know whether seatunnel will change their startup script in the future or not. So I think we should remove this enum and just use the startup script name from front-end. please notice we do not ask users manually enter the startup script name because we provide drop-down box for it. Add this just for reducing the code change scope when startup script adding/changing

Copy link
Member Author

Choose a reason for hiding this comment

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

for reviewer, we have a discussion in #14131 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

The new scream shot look like this
seatunnel

return new SeatunnelEngineTask(taskRequest);
assert seatunnelParameters != null;
String startupScript = seatunnelParameters.getStartupScript();
if (startupScript.contains(STARTUP_SCRIPT_SPARK)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

change from equal enums to startup script contain specific string or not

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2023

Codecov Report

Merging #14129 (359e26c) into dev (03ff78f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 359e26c differs from pull request most recent head 6a3dcb8. Consider uploading reports for the commit 6a3dcb8 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14129      +/-   ##
============================================
- Coverage     38.39%   38.38%   -0.01%     
- Complexity     4456     4458       +2     
============================================
  Files          1223     1223              
  Lines         42543    42543              
  Branches       4715     4713       -2     
============================================
- Hits          16334    16332       -2     
  Misses        24410    24410              
- Partials       1799     1801       +2     

see 2 files with indirect coverage changes

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

* fix error when save task seatunnel
* support all version of seatunnel startup script
  by using string instead of engine enum
* Remove unnecessary code in task seatunnel
* fix error when save task seatunnel
* support all version of seatunnel startup script
  by using string instead of engine enum
* Remove unnecessary code in task seatunnel

close apache#14114
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@zhongjiajie
Copy link
Member Author

@zhuangchong do you have time to take a look with this PR?

Copy link
Contributor

@zhuangchong zhuangchong left a comment

Choose a reason for hiding this comment

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

+1

@zhuangchong zhuangchong merged commit 56b0f91 into apache:dev May 19, 2023
@zhongjiajie zhongjiajie deleted the b-task-st branch May 19, 2023 03:13
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
3.1.x for 3.1.x version backend document UI ui and front end related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [[Task Plugin] seatunnel engine is not available in V3.1.6.
3 participants