feat: enable stale permission GC #223
Conversation
| @@ -102,6 +102,11 @@ service Query { | |||
| option (google.api.http).get = "/greenfield/storage/policy_for_group/{resource}/{principal_group_id}"; | |||
| } | |||
|
|
|||
| // Queries a policy by policy id | |||
| rpc QueryPolicyById(QueryPolicyByIdRequest) returns (QueryPolicyByIdResponse) { | |||
There was a problem hiding this comment.
Currently enabe this api for e2e, might need to hide this api
x/storage/keeper/keeper.go
Outdated
| k.permKeeper.ForceDeleteGroupPolicyForResource(ctx, resource.RESOURCE_TYPE_OBJECT, id) | ||
| deletedCount++ | ||
| // reaches the deletion limit during current endblocker | ||
| if deletedCount > maxCleanup { |
There was a problem hiding this comment.
I finally know why forget the limit count above. Actually, we should limit the kv count per block. not the resource count.
| } | ||
|
|
||
| // the specified block height(iterator-key)'s stale resource permission metadata is purged | ||
| if deleteInfo.IsEmpty() { |
There was a problem hiding this comment.
If the delete info is not empty, but some permission info associated with certain resources has already been deleted, it still needs update and persist.
x/storage/abci.go
Outdated
| @@ -13,6 +13,7 @@ func BeginBlocker(ctx sdk.Context, keeper k.Keeper) { | |||
| keeper.ClearDiscontinueObjectCount(ctx) | |||
| keeper.ClearDiscontinueBucketCount(ctx) | |||
| } | |||
| keeper.InitDeleteInfo(ctx) | |||
There was a problem hiding this comment.
Why needs store this empty key if there is no deleted resource in this block.
There was a problem hiding this comment.
yeah actually this is redundant, because when appending resource ids to deleteInfo, we are still checking the key existence.
alexgao001
force-pushed
the
feat-storage-permission-gc
branch
from
c9aebba
to
48f3a40
Compare
alexgao001
force-pushed
the
feat-storage-permission-gc
branch
from
b3fe221
to
2b6af1b
Compare
alexgao001
force-pushed
the
feat-storage-permission-gc
branch
from
377ca55
to
ee34014
Compare
| // EventStalePolicyCleanup is emitted when specified block height's stale policies is garbage collected | ||
| message EventStalePolicyCleanup { | ||
| int64 blockNum = 1; | ||
| DeleteInfo delete_info =2; |
x/storage/keeper/keeper.go
Outdated
| deleteStalePoliciesPrefixStore.Set(iterator.Key(), k.cdc.MustMarshal(deleteInfo)) | ||
| return | ||
| } | ||
| k.permKeeper.ForceDeleteGroupMembers(ctx, id) |
There was a problem hiding this comment.
Why don't group members has a maximum cleanup count limit?
|
|
||
| func (k Keeper) GarbageCollectResourcesStalePolicy(ctx sdk.Context) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| deleteStalePoliciesPrefixStore := prefix.NewStore(store, types.DeleteStalePoliciesPrefix) |
There was a problem hiding this comment.
Too many duplicated code here, I think we can improve it.
x/storage/types/types.go
Outdated
| if di == nil { | ||
| return true | ||
| } | ||
| if di.BucketIds == nil || (di.BucketIds != nil && len(di.BucketIds.Id) == 0) { |
There was a problem hiding this comment.
di.BucketIds != nil seems unnecessary.
Description
When resource bucket and object are deleted by user, its associated stale policies linked to personal accounts or groups need to be garbage collected. For resource group, its associated stale policy with personal accounts, and all group members meta will be garbage collected. these are done in each endblock auto execution.
Rationale
Get rid of useless stale permission data
Example
NA
Changes
Notable changes: