dimas-b commented on code in PR #3267:
URL: https://github.com/apache/polaris/pull/3267#discussion_r2643518123
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStoreManagerFactory.java:
##########
@@ -198,20 +198,28 @@ BaseResult purgeRealm(String realmId) {
var nextStatus =
switch (existing.status()) {
- case CREATED, LOADING, INITIALIZING, INACTIVE -> PURGING;
+ case CREATED, LOADING, INITIALIZING, INACTIVE, PURGING ->
+ // PURGING is the state telling the maintenance service to
delete the realm data.
+ PURGING;
case ACTIVE -> INACTIVE;
- case PURGING ->
- // TODO this state should really happen during maintenance!!
+ case PURGED ->
+ // The PURGED state is also handled by the maintenance service.
PURGED;
- case PURGED -> PURGED;
};
+ if (nextStatus == existing.status()) {
+ // No status change (PURGING/PURGED), stop here.
+ // Maintenance service will handle the actual deletion of the realm
data.
+ break;
+ }
+
var update =
RealmDefinition.builder().from(existing).status(nextStatus).build();
var updated = realmManagement.update(existing, update);
- if (updated.status() == PURGED) {
- realmManagement.delete(updated);
+ if (updated.status() == PURGING || updated.status() == PURGED) {
Review Comment:
Yes, I understand the current state transitions :) however, since in this
piece of code the intent is to transition from any non-purged state to
`PURGING`, having state transition restrictions in `RealmManagementImpl` seems
artificial. This code will transition to the state it wants anyway, so why
impede this transition and make it go through a loop?
This is a tangential comment based on general observations... not a blocker
for this PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
