-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(glue): restore notifyDelayAfter across different job types #33842
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
Conversation
- move `notifyDelayAfter` prop to `JobProps` - refactor to reduce code duplication - move `jobRunQueuingEnabled` prop to `JobProps` - introduce `Job.setupJobResource` utility static method to handle setting up the `CfnJob` for all job types - refactor all job types to use `Job.setupJobResource` while passing the overrides/customizations relevant to each job type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #33842 +/- ##
=======================================
Coverage 82.38% 82.38%
=======================================
Files 120 120
Lines 6955 6955
Branches 1173 1173
=======================================
Hits 5730 5730
Misses 1120 1120
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I notice this PR makes significant changes across multiple job types and includes high volume code refactoring. While the changes are well-organized, would it be possible to split this into smaller, more focused PRs? This would make the review process more manageable. |
Hi @ozelalisen I had missed your comment, as it took quite sometime between submitting the PR your interaction. Due to current commitments, I'm not sure I'd be able to manage breaking it down into smaller more focused PRs (and apologies for the large PR - but this was part of following up to a large breaking refactor already in glue alpha). I also see that with time passing, there's now conflicts to resolve. I'll try to do that - but again with current commitments am not sure how much time I can dedicate to this. |
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Related to #33356
Closes #33839
Reason for this change
address the regression of "notifyDelayAfter got removed from PySparkEtlJob props"
Description of changes
notifyDelayAfter
prop toJobProps
jobRunQueuingEnabled
prop toJobProps
Job.setupJobResource
utility static method to handle setting up theCfnJob
for all job typesJob.setupJobResource
while passing the overrides/customizations relevant to each job typeextraJars
,extraJarsFirst
, andextraFiles
props from the different spark job props intoSparkJobProps
Describe any new or updated permissions being added
N/A
Description of how you validated changes
unit and integ tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license