Conversation
MikeDombo
reviewed
Jan 20, 2026
| */ | ||
| private void handleCurrentStateUninstalled(Optional<State> desiredState) { | ||
| //do nothing, unless its reinstall | ||
| if (State.NEW.equals(desiredState.get())) { |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
It appears that you are using Optional value which can hold either a value or not. The value held in the Optional can be accessed using the get() method, but it will throw a NoSuchElementException if there is no value present. To avoid the exception, calling the isPresent() or ifPresent() or !isEmpty() method should always be done before any call to get(). Alternatively, other methods such as orElse(...), orElseGet(...) or orElseThrow(...) can be used to specify what to do with an empty Optional.
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
from
c7752b1 to
e847670
Compare
MikeDombo
reviewed
Jan 27, 2026
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
4 times, most recently
from
811ec4b to
1b25981
Compare
|
Binary incompatibility detected for commit 48fcfc8. 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 48fcfc8 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 48fcfc8 |
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
2 times, most recently
from
fe9ca4e to
79f4f01
Compare
alter-mage
reviewed
Jan 29, 2026
| } | ||
|
|
||
| void setUninstall(boolean b) { | ||
| requestedUninstall.set(b); |
Member
There was a problem hiding this comment.
nit: b is always true, right?
Member
Author
There was a problem hiding this comment.
b is not always true. our usage right now is to only call true, yes.
Member
There was a problem hiding this comment.
Right, does not make sense to pass the variable b if the setUninstall method always sets the requestedUninstall to true.
Member
Author
There was a problem hiding this comment.
sure I can flatten this. I was aligning this with how isClosed is implemented, but it seems like isClosed is also access outside of this class.
alter-mage
reviewed
Jan 29, 2026
| logger.atInfo(MERGE_CONFIG_EVENT_KEY).kv("service-to-remove", servicesToRemove).log("Removing services"); | ||
|
|
||
| // Request uninstall for each service before closing | ||
| for (GreengrassService service : ggServicesToRemove) { |
Member
There was a problem hiding this comment.
to me it makes more sense to stop a service before uninstalling it. Imo, this uninstall loop should come after the next loop.
This will also simplify your requestUninstall method in Lifecycle
Member
Author
There was a problem hiding this comment.
This is what I thought initially too, but there is a situation where a component can close without trying to uninstall. specifically in version upgrades. So we need a way to differentiate before the life cycle finishes. simply calling close before uninstall would cause the component to exit out of the lifecycle loop and not attempt the uninstall step.
Member
There was a problem hiding this comment.
I thought we were not doing uninstall during a component version update in this iteration. Is that not true?
Member
Author
There was a problem hiding this comment.
yes, so this is specifically designed to guard against that.
there are also other situations where a component can close, without uninstalling.
alter-mage
reviewed
Jan 29, 2026
...va/com/aws/greengrass/integrationtests/lifecyclemanager/GenericExternalServiceIntegTest.java
Show resolved
Hide resolved
alter-mage
reviewed
Jan 30, 2026
| State prevState = getState(); | ||
| while (!(isClosed.get() && getState().isClosable())) { | ||
| // if uninstall is requested, the wait for uninstalled state, else see if is closable | ||
| while (requestedUninstall.get() |
Member
There was a problem hiding this comment.
why do you need the additional check?
Member
Author
There was a problem hiding this comment.
this comes from the logic that we can have both closing components and uninstalling components. closing component that is trying to uninstall should not exit out of this loop, instead should run until the state is UNINSTALLED. Closing component can just exit when ever it reaches on of the satisfied states.
Member
There was a problem hiding this comment.
Is there a race condition here where a component is marked as closed but not yet marked for uninstall?
Member
Author
There was a problem hiding this comment.
if it marked for closed not for uninstall then we will simply actually close it when it reaches finish.
Member
Author
There was a problem hiding this comment.
uninstalling is a subset of closing. so if its only closing, we will exit when it reaches finished/broken, but if its closing + uninstalling, we will only exit at uninstalled.
alter-mage
reviewed
Jan 30, 2026
alter-mage
reviewed
Jan 30, 2026
saranyailla
reviewed
Jan 30, 2026
src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java
Outdated
Show resolved
Hide resolved
saranyailla
reviewed
Jan 30, 2026
| State prevState = getState(); | ||
| while (!(isClosed.get() && getState().isClosable())) { | ||
| // if uninstall is requested, the wait for uninstalled state, else see if is closable | ||
| while (requestedUninstall.get() |
Member
There was a problem hiding this comment.
What's the behavior here when an uninstalled service is rolled back from a failed deployment?
Member
Author
There was a problem hiding this comment.
a reinstall/install will be requested which will reset this boolean. then the install script will run again.
saranyailla
reviewed
Jan 30, 2026
| /** | ||
| * Handle UNINSTALLING state - execute uninstall script and exit lifecycle thread. | ||
| */ | ||
| private void handleCurrentStateUninstalled(Optional<State> desiredState) { |
Member
There was a problem hiding this comment.
The UNINSTALLED service instance must be removed from GG context in this case. This is handled during the deployment execution itself. So, we don't need to handle the NEW state of the service instance here. This is similar to handling FINISHED state.
Member
Author
There was a problem hiding this comment.
This is to account for the edge case where a reinstall is requested when a new deployment (after the one to remove the component) comes in with a new version of the component. so we do not stay in this limbo uninstalled state. you can see something similar in here
Member
There was a problem hiding this comment.
Can we reset the requestedUninstall flag here?
saranyailla
reviewed
Jan 30, 2026
| */ | ||
| @SuppressWarnings("PMD.MissingBreakInSwitch") | ||
| private void serviceTerminatedMoveToDesiredState(@Nonnull State desiredState) { | ||
| if (requestedUninstall.get()) { |
Member
There was a problem hiding this comment.
requestedUninstall.get() returns True for a service that is already in UNINSTALLED state. So, reporting the state as UNINSTALLING is not correct.
Member
Author
There was a problem hiding this comment.
it returns true for when a component is attempting to uninstall, this is true for how isClosed is interpreted too. so its valid here to report uninstalling. What I should do is to ensure that the state is in FINISHED before doing so.
saranyailla
reviewed
Jan 30, 2026
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
2 times, most recently
from
b950bd4 to
e0768ab
Compare
saranyailla
reviewed
Feb 6, 2026
src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java
Outdated
Show resolved
Hide resolved
saranyailla
reviewed
Feb 6, 2026
| } | ||
|
|
||
| /** | ||
| * Handle UNINSTALLING state - execute uninstall script and exit lifecycle thread. |
alter-mage
previously approved these changes
Feb 6, 2026
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
from
e0768ab to
9e5bbec
Compare
muboheng
force-pushed
the
add_uninstall_to_recipe
branch
from
9e5bbec to
48fcfc8
Compare
saranyailla
reviewed
Feb 12, 2026
| @Override | ||
| protected void uninstall() { | ||
| try (LockScope ls = LockScope.lock(lock)) { | ||
| logger.atInfo().log("Shutdown initiated"); |
saranyailla
approved these changes
Feb 12, 2026
alter-mage
approved these changes
Feb 13, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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:
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.