[Feature-18070][Task] Add Amazon EMR Serverless task plugin#18069
[Feature-18070][Task] Add Amazon EMR Serverless task plugin#18069SbloodyS merged 29 commits intoapache:devfrom
Conversation
norrishuang
requested review from
EricGao888,
Gallardot,
SbloodyS,
caishunfeng,
ruanwenjun and
songjianet
as code owners
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md) |
norrishuang
changed the title
SbloodyS
left a comment
SbloodyS
left a comment
There was a problem hiding this comment.
Please add api-test or e2e for this. @norrishuang
Comprehensive unit tests have already been included for the EMR Serverless task plugin, covering job submission, state polling, success/failure/cancellation handling, failover recovery, parameter validation, and invalid input scenarios. Since this task plugin depends on AWS EMR Serverless, running api-test or e2e in the CI Docker environment would require AWS credentials and a running EMR Serverless application. I'm happy to add an api-test or e2e if there is a recommended approach for handling AWS authentication in CI. Could you share any guidance on this? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thank you for the feedback @SbloodyS! I have enhanced the unit tests to provide comprehensive coverage of the EMR Serverless task plugin. The test suite now includes 15 test cases covering:
The tests use Mockito to mock |
|
Unit testing is not enough. You can refer to |
|
Thank you for the guidance @SbloodyS! I have added an api-test for the EMR Serverless task plugin. Since this plugin depends on AWS EMR Serverless (a cloud service), running actual e2e tests in CI would require real AWS credentials and a running EMR Serverless application. To solve this, I used WireMock to mock the AWS EMR Serverless HTTP API — it's open-source and works entirely offline. What was added (commit: norrishuang/dolphinscheduler@b96944c):
Please let me know if any adjustments are needed. |
|
Yes. Using |
|
Hi @SbloodyS, I noticed the OWASP Dependency Check CI has been failing on the |
You can just ignore it for now. |
|
Hi @SbloodyS, thanks for the review comments! I've addressed all three points:
All CI checks are now passing. Please take another look when you have a chance! |
- Replace deprecated PropertyNamingStrategy.UpperCamelCaseStrategy with PropertyNamingStrategies.UPPER_CAMEL_CASE (fixes SonarCloud warning) - Remove redundant applicationId field; read directly from emrServerlessParameters - Store only jobRunId in appIds (applicationId always available from parameters) - Simplify failover recovery: jobRunId = getAppIds() directly - Remove @SInCE dev-SNAPSHOT javadoc tag - Update test to use new appIds format (jobRunId only)
|
Hi @SbloodyS, thank you for the detailed review! I've addressed all the feedback in the latest commit:
All 15 unit tests pass. Please take another look when you have a chance. |
|
There are still many unaddressed comment. @norrishuang |
- Use JobRunState enum constants in WAITING_STATES set - Replace switch/case with JobRunState enum in mapStateToExitCode() - Update EmrServerlessTaskTest to use JobRunState enum - Apply spotless format fixes - All 15 unit tests pass
- Remove unnecessary 'public' modifiers from @test and @beforeeach methods (S5786) - Add no-op comments to empty TaskCallBack methods (S1186) - Remove unnecessary 'throws Exception' declarations where not needed (S1130) - All 15 unit tests pass
|
Hi @SbloodyS, I've addressed all the remaining review comments. Replaced hardcoded state strings with Could you take another look? Thanks! |
- Add WorkflowInstancePage to EmrServerlessTaskAPITest - Poll workflow instance state in success test: assert final state is SUCCESS - Poll workflow instance state in failed test: assert final state is FAILURE/STOP - Pattern follows DependentTaskAPITest
- Remove deprecated PropertyNamingStrategies.UPPER_CAMEL_CASE usage - Remove empty javadoc line - Fix infinite loop in API tests with 120s timeout - Reduce polling interval from 5s to 2s - Add explicit workflow instance final state assertions Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove deprecated PropertyNamingStrategies.UPPER_CAMEL_CASE usage - Remove empty javadoc line - Fix infinite loop in API tests with 120s timeout - Reduce polling interval from 5s to 2s - Add explicit workflow instance final state assertions
- Use throws Exception instead of try-catch to propagate errors clearly - Replace while(true) with for-loop (60 iterations x 2s = 120s timeout) - Use boolean completed flag with explicit assertTrue assertion - Fail immediately with state info when unexpected terminal state reached
|
|
Awesome work, congrats on your first merged pull request! |
3 participants
Was this PR generated or assisted by AI?
YES. The implementation was assisted by AI (Claude) for code generation, with human review, testing and verification on a real AWS EMR Serverless environment.
Purpose of the pull request
Add a new task plugin for Amazon EMR Serverless, enabling users to submit, monitor, and cancel Spark/Hive jobs on EMR Serverless applications directly from DolphinScheduler workflows.
Unlike the existing EMR on EC2 task plugin which manages EC2-based clusters, EMR Serverless is a serverless runtime that requires no cluster infrastructure management and automatically scales compute resources on demand.
Close 18070
Brief change log
Backend (new module:
dolphinscheduler-task-emr-serverless)EmrServerlessTask— extendsAbstractRemoteTask, implements submit/track/cancel lifecycle via AWS SDK v1 (StartJobRun,GetJobRun,CancelJobRun)EmrServerlessParameters— task parameter model (applicationId, executionRoleArn, jobName, startJobRunRequestJson)EmrServerlessTaskChannel/EmrServerlessTaskChannelFactory— SPI registration via@AutoService, registered asEMR_SERVERLESSEmrServerlessTaskException— dedicated exception classaws.emr.*config fromaws.yaml, falls back toDefaultAWSCredentialsProviderChainappIds(jobRunId)Frontend
use-emr-serverless.ts(fields) — form fields for Application Id, Execution Role Arn, Job Name, StartJobRunRequest JSON editoruse-emr-serverless.ts(tasks) — task model definitionDocumentation
docs/docs/zh/guide/task/emr-serverless.mddocs/docs/en/guide/task/emr-serverless.mdVerify this pull request
This change added tests and can be verified as follows:
EmrServerlessTaskTestwith 11 unit tests covering: success/failed/cancelled lifecycle, full state chain, submit error handling, null GetJobRun response, cancel with/without jobRunId, failover recovery, parameter validation, and invalid JSON handling.