-
Notifications
You must be signed in to change notification settings - Fork 30
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
[Fetch Migration] Suppress exit code for graceful termination (SIGTERM) #478
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #478 +/- ##
============================================
+ Coverage 73.55% 73.76% +0.20%
+ Complexity 1182 1180 -2
============================================
Files 124 124
Lines 4886 4886
Branches 439 439
============================================
+ Hits 3594 3604 +10
+ Misses 998 988 -10
Partials 294 294
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Is there an integ test in place that runs DataPrepper and confirms that the error code and this codebase works as expected? If so, LGTM. If not, it would be really nice to have that to ensure that we'll know when DataPrepper changes and we need to make an update.
return migration_monitor.run(migration_monitor_params, proc) | ||
return_code = migration_monitor.run(migration_monitor_params, proc) | ||
# Suppress non-zero return code for graceful termination (SIGTERM) | ||
if return_code == 143: |
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.
A citation to the DataPrepper code (github) showing where/why it returns 143 would be great. 143 seems like it could be a fragile number to pin on. This should also be a constant to document it a bit more.
I guess DataPrepper can also just run in "one-off" mode where it exits after a task is done (and then exits with code 0)?
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.
143
is the standard exit code for a SIGTERM
signal to indicate a graceful shutdown of the process. This behavior isn't managed by Data Prepper but rather by the JVM.
I wasn't able to find any system libraries in Python that have this declared as a constant. IMO a test case isn't needed for this behavior since it is the expected behavior for any Linux process. Let me know if you disagree?
7d57793
to
190954e
Compare
The orchestrator now suppresses exit code 143 if it is returned by the migration monitor, since this is a result of a workaround for opensearch-project/data-prepper#3141 . Unit tests have been added to cover this scenario. This change also ensures that a return_code is always returned by the function, and updates some stale comments. Signed-off-by: Kartik Ganesh <[email protected]>
190954e
to
0bce309
Compare
Description
The orchestrator now suppresses exit code
143
(graceful termination following aSIGTERM
) if it is returned by the migration monitor, since this is a result of a workaround for opensearch-project/data-prepper#3141Unit tests have been added to cover this scenario. This change also ensures that a return_code is always returned by the function, and updates some stale comments.
Testing
Unit test coverage:
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.