-
Notifications
You must be signed in to change notification settings - Fork 535
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
Container Rehydration from a snapshot taken from detached container. #3061
Conversation
packages/framework/aqueduct/src/containerRuntimeFactories/baseContainerRuntimeFactory.ts
Outdated
Show resolved
Hide resolved
Ping |
@@ -193,7 +194,8 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i | |||
options: any, | |||
scope: IFluidObject, | |||
loader: Loader, | |||
source: IFluidCodeDetails, | |||
codeDetails: IFluidCodeDetails | undefined, | |||
snapshot: ISnapshotTree | undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's easy to differentiate interfaces, I'd rather have single IFluidCodeDetails | ISnapshotTree argument.
Or a type that differentiates them:
{ create: true, code: IFluidCodeDetails } | { create: false, snapshot: ISnapshotTree}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chose this one: { create: true, code: IFluidCodeDetails } | { create: false, snapshot: ISnapshotTree}. It is more explicit, infact it was like this before but later moved to other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like like this type. i'd rather just have separate methods. also, can the code proposal be updated on a detached container? i think we will want to support this if we don't
In reply to: 479472885 [](ancestors = 479472885)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you basically already have two methods anyway. i don't see a whole lot of value in coupling these
In reply to: 483120067 [](ancestors = 483120067,479472885)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 methods one of which load from codeDetails and the other which load from snapshot are private methods. Loader calls create method on container which is public if it wants to create a detached container either from snapshot or code details. So this type is just used here. Aesthetically this looks good to me:
{ create: true, code: IFluidCodeDetails } | { create: false, snapshot: ISnapshotTree}
Code can be proposed again by the quorum, isn't it? I mean can we update the code and load from snapshot at the same time? I don't think so.
this.attached = true; | ||
} | ||
|
||
private get services() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pattern is error prone where we have both a private member and a property. there is a Lazy class in common\lib\common-utils\src\lazy.ts that is a better pattern for delay initialization like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we were depending on previous version of common utils package where the lazy class is not exported. So i have exposed that here and then will follow up with another Pr to use that class here, otherwise this Pr will become huge.
this.dirtyFn = () => { dirtyFn(id); }; | ||
} | ||
|
||
public async getChannel(): Promise<IChannel> { | ||
if (this.channel === undefined) { | ||
this.channel = await this.loadChannel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.channel = await this.loadChannel(); [](start = 12, length = 40)
You should probably use a promise (channelP) paradigm here similar to how the remote context does.
* Creates a new container using the specified snapshot but in an unattached state. While unattached all | ||
* updates will only be local until the user explicitly attaches the container to a service provider. | ||
*/ | ||
rehydrateDetachedContainerFromSnapshot(snapshot: ISnapshotTree): Promise<IContainer>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rehydrateDetachedContainerFromSnapshot [](start = 4, length = 38)
nit: Consider rehydrateContainer
or rehydrateContainerFromSnapshot
. I don't think we should expose detached keyword here.
Not related to this PR, but I think createDetachedContainer should also be just called createContainer. Now that detached container is (almost) used everywhere, the word "detached" doesn't add any extra value.
Reopen. |
Issue: #2692.
Solution:
1.) Loader's createDetachedContainer now takes either iFluidCodeDetails or snapshot. If snapshot is provided, we load from the snapshot.
2.) We keep ripping through the snapshot at container runtime layer and component runtime layer and create local component and channels from the snapshot.
3.) We support delay loading when we load from the snapshot. So when we take the summary for the container to attach, if the component/dds is loaded then we ask them for the summary otherwise we just use the previous summary from which it loaded.
4.) At dds level, we have added a loadLocal api where we load the dds from snapshot but we don't assign it services.
5.) All the components loaded from snapshot are declared as bind but not attached.