feat: add ability to configure tes error cache expire time #1751
+149
−31
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Binary incompatibility detected for commit 4698da7. com.aws.greengrass.tes.CredentialRequestHandler is binary incompatible and is source incompatible because of FIELD_REMOVED Produced by binaryCompatability.py |
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 4698da7 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 4698da7 |
saranyailla
reviewed
Nov 21, 2025
src/main/java/com/aws/greengrass/tes/CredentialRequestHandler.java
Outdated
Show resolved
Hide resolved
saranyailla
reviewed
Nov 24, 2025
saranyailla
reviewed
Nov 24, 2025
saranyailla
reviewed
Nov 24, 2025
saranyailla
reviewed
Nov 24, 2025
… avoid multiple restarts
rauthsuj
force-pushed
the
tes-configure
branch
from
7b3fd9b to
732599d
Compare
saranyailla
reviewed
Nov 25, 2025
Comment on lines
+90
to
+93
| if (node != null && (node.childOf(PORT_TOPIC) | ||
| || node.childOf(CLOUD_4XX_ERROR_CACHE_TOPIC) | ||
| || node.childOf(CLOUD_5XX_ERROR_CACHE_TOPIC) | ||
| || node.childOf(UNKNOWN_ERROR_CACHE_TOPIC))) { |
Member
There was a problem hiding this comment.
Nit: We don't need to execute this callback if the what happened events are irrelevant to this. For eg, see how we don't take an action if the what happened event doesn't show a change in value.
saranyailla
previously approved these changes
Nov 25, 2025
Member
|
We had an offline discussion about if the component should enter errored state when configuration is not valid, instead of printing an error but proceeding to succeed operation by using the minimum value rather than the configured value. We agreed it is preferable to enter errored state. So that the component provides feedback to greengrass about the configuration not being valid, let greengrass decide how to proceed, and provide immediate feedback to the customer about the invalid configuration. Currently there is only one way for TES config to be updated, that is through deployments. In this case greengrass can fail the deployment and roll back to a previously working state if configured to do so. In the future if we introduce the ability for TES config to be updated outside of a deployment (i.e. during runtime) (e.g. by calling UpdateConfiguration for TES from some custom component), then it seems like entering errored state would cause the device to become unhealthy and not auto-recover at that time. This problem is left to be addressed in the future. |
|
|
||
| private Instant getExpiryPolicyForErr(int statusCode) { | ||
| int expiryTime = UNKNOWN_ERROR_CACHE_IN_MIN; // In case of unrecognized cloud errors, back off | ||
| int expiryTime = unknownErrorCacheInSec; // In case of unrecognized cloud errors, back off |
Member
There was a problem hiding this comment.
nit: it was already like this, but expiryDuration is more appropriate than expiryTime
| logger.atInfo("tes-config-change") | ||
| .kv("node", node).kv("why", why) | ||
| .log("Restarting TES server due to config change"); | ||
| requestRestart(); |
Member
There was a problem hiding this comment.
I believe you should be able to change the error cache time during runtime and without restart. I don't see why not. This would change the subscription handler logic to only request restart if the port changes. This would be useful because TES restarts cause a lot of dependent component restarts, which we want to avoid if possible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Add ability for users to configure TES error cache expire time
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.