Removing atomic handling for ManagedVSR and cleaning up state transitions #20120
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
❌ Gradle check result for 8c44afb: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
raghuvanshraj
force-pushed
the
bugfix/memory-leak-pr
branch
from
8c44afb to
4c13cf5
Compare
|
❌ Gradle check result for 4c13cf5: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 3a2b211: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
raghuvanshraj
force-pushed
the
bugfix/memory-leak-pr
branch
from
3a2b211 to
f813cbc
Compare
…ions Signed-off-by: Raghuvansh Raj <raghraaj@amazon.com>
|
❌ Gradle check result for f813cbc: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Raghuvansh Raj <raghraaj@amazon.com>
raghuvanshraj
force-pushed
the
bugfix/memory-leak-pr
branch
from
f813cbc to
5acf690
Compare
|
❌ Gradle check result for 5acf690: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| // If already CLOSED, do nothing (idempotent) | ||
| if (state == VSRState.CLOSED) { | ||
| return; | ||
| } | ||
|
|
||
| // If ACTIVE, must freeze first | ||
| if (state == VSRState.ACTIVE) { | ||
| throw new IllegalStateException(String.format( | ||
| "Cannot close VSR %s: VSR is still ACTIVE. Must freeze VSR before closing.", id)); | ||
| } | ||
|
|
||
| // If FROZEN, transition to CLOSED | ||
| if (state == VSRState.FROZEN) { | ||
| moveToClosed(); | ||
| } else { | ||
| // This should never happen with current states, but defensive programming | ||
| throw new IllegalStateException(String.format( | ||
| "Cannot close VSR %s: unexpected state %s", id, state)); |
There was a problem hiding this comment.
Looks like we need a dedicated centralised class VSRState for state machine validation
private static final Set<VSRState> ACTIVE_VALID_TRANSITIONS = EnumSet.of(FROZEN, CLOSED);
private static final Set<VSRState> FROZEN_VALID_TRANSITIONS = EnumSet.of(FLUSHING, CLOSED);
private static final Set<VSRState> FLUSHING_VALID_TRANSITIONS = EnumSet.of(CLOSED);
private static final Set<VSRState> CLOSED_VALID_TRANSITIONS = EnumSet.noneOf(VSRState.class);
private Set<VSRState> getValidTransitions() {
switch (this) {
case ACTIVE:
return ACTIVE_VALID_TRANSITIONS;
case FROZEN:
return FROZEN_VALID_TRANSITIONS;
case FLUSHING:
return FLUSHING_VALID_TRANSITIONS;
case CLOSED:
return CLOSED_VALID_TRANSITIONS;
default:
throw new IllegalStateException("Unknown state: " + this);
}
}
There was a problem hiding this comment.
I didn't see a usecase for the FLUSHING state as of now, hence I removed it. Between ACTIVE, FROZEN and CLOSED, we have only a small set of valid transitions, do you think we need the FLUSHING state?
There was a problem hiding this comment.
Also, if we do introduce a FLUSHING state, should we allow FROZEN to go to CLOSED? Ideally every FROZEN VSR should first go to FLUSHING state then CLOSED.
2 participants
Description
This change simplifies the lifecycle of a ManagedVSR. Close handling and state transitions are made more robust. Fixes a memory leak in the VSR rotation framework.
Related Issues
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.