[Object spilling] Refactor raylet to add a local object manager class #11647
Conversation
stephanie-wang
changed the title
|
|
||
| private: | ||
| /// Delete freed objects. | ||
| void Flush(); |
There was a problem hiding this comment.
Are we planning to spill those freed objects later within this method? The function name is a little confusing to me (because as far as I know flush means you'd like to move buffered objects onto the durable storage).
There was a problem hiding this comment.
This actually just triggers the callback for freed objects. It doesn't have anything to do with spilling. I'll update the name and description.
| if (!status.ok()) { | ||
| RAY_LOG(WARNING) << "Worker failed. Unpinning object " << object_id; | ||
| } | ||
| RAY_LOG(DEBUG) << "Unpinning object " << object_id; |
There was a problem hiding this comment.
Can we take out these logic into a different private method like FreePinnedObjects? It was a little confusing to me that Pin objects method also unpin objects.
| ASSERT_EQ(num_times_fired, 1); | ||
| for (size_t i = 0; i < object_ids.size(); i++) { | ||
| ASSERT_EQ(object_table.object_urls[object_ids[i]], urls[i]); | ||
| } |
There was a problem hiding this comment.
It looks like AsyncRestoreSpilledObject is not tested here. Are you planning to add it in the follow-up PR?
There was a problem hiding this comment.
Oh I didn't add it because there isn't that much to test...but yeah let me add a unit test for it.
ericl
added
the
@author-action-required
Co-authored-by: Eric Liang <ekhliang@gmail.com>
stephanie-wang
removed
the
@author-action-required
Why are these changes needed?
This adds a local object manager class with unit tests to the raylet to improve code maintainability. This class is responsible for maintaining any primary object copies in the local plasma store, freeing these objects when appropriate, and coordinating spilling and restoring objects to and from external storage.
Checks
scripts/format.shto lint the changes in this PR.