-
Notifications
You must be signed in to change notification settings - Fork 215
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
ENH: keep retrying when getSecretValue fails #4102
ENH: keep retrying when getSecretValue fails #4102
Conversation
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
String.format("Unable to retrieve secret: %s", | ||
awsSecretManagerConfiguration.getAwsSecretId()), e); | ||
GetSecretValueResponse getSecretValueResponse; | ||
while (true) { |
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.
I don't think having an infinite loop this deep is a good idea. We should manage this in a thread specifically for this purpose.
@@ -107,17 +108,26 @@ public void refresh(String secretConfigId) { | |||
LOG.info("Finished retrieving latest secret in aws:secrets:{}.", secretConfigId); | |||
} | |||
|
|||
@SkipTestCoverageGenerated |
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 is intended for generated code and maybe boilerplate. This code is significant and should be tested.
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.
The motivation to put this is due to coverage shortage on the exception here:
try {
Thread.sleep(waitTimeInMillis);
} catch (InterruptedException ex) {
throw new RuntimeException(ex);
}
but it might no avoidable with some logic improvement
Signed-off-by: George Chen <[email protected]>
@dlvenable I ended up floating up the infinite retrying to the constructor level such that the refresh method call will not be blocked. Currently all extension loading is prerequisite for pipeline plugin construction. So it will block overall pipeline creation:
|
Signed-off-by: George Chen <[email protected]>
Close this PR as neither pipeline nor data prepper server would spin up due to this blocking behavior on retrieving secrets. Thus it adds no particular value to Data Prepper spinning up. |
created issue: #4122 |
Description
This PR makes the enhancement that data prepper does not crash under the event of not able to retrieving the secrets. It will keep on retrying until user fixes the secret manager backend.
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
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.