storage: use StorageResponse only for cluster protocol #30720
Conversation
petrosagg
force-pushed
the
reserve-storage-responses
branch
from
bc41ec3 to
461e99d
Compare
The controller was previously generating synthetic StorageResponses for itself via a channel in order to delay shard finalization. However, shard finalization is no longer performed by the storage controller (looks like it moved to StorageCollections). So this PR removes this mechanism and instead drops the state immediately. Signed-off-by: Petros Angelatos <[email protected]>
petrosagg
force-pushed
the
reserve-storage-responses
branch
from
461e99d to
c3cf99d
Compare
| for id in pending_collection_drops { | ||
| self.collections | ||
| .remove(&id) | ||
| .expect("list populated after checking that self.collections contains it"); | ||
| } |
There was a problem hiding this comment.
The documentation of Controller::collections has this to say:
/// This collection only grows, although individual collections may be rendered unusable.
/// This is to prevent the re-binding of identifiers to other descriptions.
Looks like this also wasn't true before, so the comment is wrong. On the other hand, it seems like Controller::exports is never removed from, so maybe the comment should move there?
There was a problem hiding this comment.
This comment must be referring back to a very old design goal of having the storage controller permanently bind a global id with a description. This is described in Frank's architecture design doc. For that to work durable state is required but we don't have that so that comment is.. not correct.
I don't know why exports are not cleaned up, that looks like a bug to me because entries will certainly be removed from there once environment restarts and adapter never mentions the dropped exports again (because it has also forgotten about them).
I will delete that comment altogether
The controller was previously generating synthetic StorageResponses for itself via a channel in order to delay shard finalization. However, shard finalization is no longer performed by the storage controller (looks like it moved to StorageCollections). So this PR removes this mechanism and instead drops the state immediately.
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.