Convert attributor to IFluidDataStoreChannel #22120
Conversation
|
github-actions
bot
added
area: framework
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 56989e4 |
jatgarg
requested review from
a team,
pragya91,
markfields,
tyler-cai-microsoft,
kian-thompson and
rajatch-ff
and removed request for
a team
| @@ -4,10 +4,10 @@ | |||
| */ | |||
|
|
|||
| export { type IAttributor } from "./attributor.js"; | |||
| export { mixinAttributor, getRuntimeAttributor } from "./mixinAttributor.js"; | |||
There was a problem hiding this comment.
Are we going to fully remove mixinAttributor in a later change?
There was a problem hiding this comment.
All that mixinAttributor does now is override loadRuntime (yay, it doesn't have to mix in summarization stuff anymore!).
All that loadRuntime is doing inside can be flipped around and done outside/around the call to ContainerRuntime.loadRuntime, with one exception - adding the IRuntimeAttributor getter. I don't think we want that anyway - getRuntimeAttributor should be used instead.
So instead of using the mixin pattern, we can just have a helper function loadRuntimeWithAttribution which does everything that the loadRuntime override does, besides messing with the private field (which no longer applies / is needed).
I would not suggest doing this in this PR, but I would suggest/request we do it soon after. Thoughts?
There was a problem hiding this comment.
Removed the getter on the container runtime as we can use the utility method to get it. But we do need to mixin as we need to inject the registry and tracking options for the attributor before container runtime is loaded, although we did remove a lot of stuff already.
There was a problem hiding this comment.
I think what I'm saying still works though - when you have a minute, try moving ContainerRuntimeWithAttributor.loadRuntime to be a free function. It should work fine and you can remove the whole ContainerRuntimeWithAttributor class.
Then where today callers have to call mixinRuntime and then call loadRuntime on the result, they can instead just call that one function (instead of calling loadRuntime directly)
There was a problem hiding this comment.
Look here how we use it here in the tests:
The benefit of overriding the ctor is that you can just use the override while creating the runtime factory rather than having to differentiate what to use much later/down the line when the runtime is getting created?
There was a problem hiding this comment.
@Abe27342 Any suggestions here?
There was a problem hiding this comment.
Ah, interesting. Yeah that would take some untangling. Worth doing later IMO, none of this is blocking this PR, the shrinking you did on that mixin is awesome.
packages/framework/attributor/src/runtimeAttributorDataStoreFactory.ts
Outdated
Show resolved
Hide resolved
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts
Outdated
Show resolved
Hide resolved
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts
Outdated
Show resolved
Hide resolved
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * {@inheritdoc IFluidDataStoreChannel.setAttachState} | ||
| */ | ||
| public setAttachState(attachState: AttachState.Attaching | AttachState.Attached): void { |
There was a problem hiding this comment.
I wish you didn't have to write this code here... I know this area has been bug-prone before, and I'm not sure anyone will know to come fix any issues here if they are discovered in the similar code in the main datastore runtime implementation.
No suggestion here, just wanting to point it out.
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts
Outdated
Show resolved
Hide resolved
packages/framework/attributor/src/runtimeAttributorDataStoreChannel.ts
Outdated
Show resolved
Hide resolved
Description
Convert attributor to IFluidDataStoreChannel, so that it can have all the capacity of a datastore channel like send/receive ops, summarize etc.
This will be useful for custom attribution support where we will need all this functionality.
Doc link for custom attribution where we will be using this to send/receive ops: https://microsoft-my.sharepoint-df.com/:w:/p/jatgarg/EaqG3AT4CrRHpN_l_vorFf4B35C5W39g8S1VI0_syIzUuQ?e=r5YiBX
Still working on some of the test in this PR but putting out there for review.