Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all expired allocations for a client when none specified #671

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

anorth
Copy link
Member

@anorth anorth commented Sep 19, 2022

This raises a bit of a question about the return value when no allocation IDs were specified. If this were a separate method, maybe we'd return the list of removed IDs. In either case maybe we'd like to return the total datacap reclaimed. I've taken the simplest possible path here of returning nothing.

We could merge these into a return value that includes all these, like:

{
	specified_allocations: BatchReturn // only non-empty when specified
    found_allocations: Vec<AllocationID> // only non-empty when none specified
    datacap: DataCap
}

What do you think @ZenGround0 ?

Closes #606.

@ZenGround0
Copy link
Contributor

I like the proposal. I've been thinking it will work generically to use the batch info as a field to index into the others alongside it and I believe this is an example of that. The only change I would make is return a batch info with success codes for every allocation successfully in the unspecified case. This way we maintain the relationship of fields of the return value across the specified and unspecified cases.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2022

Codecov Report

❗ No coverage uploaded for pull request base (decouple-fil+@f8847e0). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head bfb41af differs from pull request most recent head 2b2d013. Consider uploading reports for the commit 2b2d013 to get more accurate results

Impacted file tree graph

@@               Coverage Diff                @@
##             decouple-fil+     #671   +/-   ##
================================================
  Coverage                 ?   84.61%           
================================================
  Files                    ?       95           
  Lines                    ?    19028           
  Branches                 ?        0           
================================================
  Hits                     ?    16100           
  Misses                   ?     2928           
  Partials                 ?        0           

@anorth
Copy link
Member Author

anorth commented Sep 19, 2022

return a batch info with success codes for every allocation successfully in the unspecified case. This way we maintain the relationship of fields of the return value across the specified and unspecified cases.

I'll do this, but it can only ever be an array of ExitCode::OK the same length as the allocation IDs. Since this is an infrequent method, the result size probably doesn't matter too much, so I'll return exactly the same info in both cases: the IDs considered and the status codes and datacap, even though some can be inferred from how the method was invoked.

(Gas will be the only limit on the amount of work this could do, with large enough state).

@anorth anorth force-pushed the anorth/removeallallocs branch from bfb41af to 2b2d013 Compare September 19, 2022 03:49
@anorth anorth requested a review from ZenGround0 September 19, 2022 07:31
@ZenGround0
Copy link
Contributor

ZenGround0 commented Sep 19, 2022

Since this is an infrequent method, the result size probably doesn't matter too much

The way batch info serializes all these ExitCode::OK's is by incrementing the value of the single integer success count field, so this should be efficient.

@anorth anorth merged commit b9f6b74 into decouple-fil+ Sep 20, 2022
@anorth anorth deleted the anorth/removeallallocs branch September 20, 2022 01:05
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove expired allocations should remove all eligible when none specified
3 participants