DDS Migration Shim #23729
Draft
DDS Migration Shim #23729
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area: dds
…to migrationSHim
…to migrationSHim
…to migrationSHim
…to migrationSHim
…to migrationSHim
github-actions
bot
added
the
area: tests
…to migrationSHim
CraigMacomber
added a commit
that referenced
this pull request
Apr 10, 2025
…24312) ## Description Tweak SharedTree tests to make less direct use of the SharedTreeClass and instead prefer its interfaces. Also reduce unnecessary use of its factory. These changes make the test suite more compatible with changes like #23729 which adjust how the kernel is used to implement the SharedObject.
Merged
CraigMacomber
added a commit
that referenced
this pull request
Apr 11, 2025
## Description Tidy up tests a bit for more type safety where practical and less direct use of SharedTree class and TreeFactory. This should reduce the impact of changes like #23729 which change how the factory and class are setup.
CraigMacomber
added a commit
that referenced
this pull request
Apr 16, 2025
## Description The SharedTree class only exists to wrap SharedTreeKernel up in a ISharedObject for use with Fluid's factories and runtimes. Thus stop using the class and factory directly so that how this adaption of the kernel into a SharedObjectKind is done is no longer implicitly depended on my tests. This enables the refactor in show this is done from #23729 to not be nearly as disruptive.
…to migrationSHim
…to migrationSHim
CraigMacomber
added a commit
that referenced
this pull request
May 12, 2025
## Description Provide `makeSharedObjectKind`, a new API for implementing ISharedObject which does not rely on subclassing. Note that all the new API and abstractions here are `@internal` and thus could be changed or removed in the future. Additionally the new logic builds SharedObjects using the old APIs so it has no compat implications for the runtime side: this is simply providing a new option for how a SharedObject like SharedTree can be factored. Currently the approach used to define a DDS is to subclass SharedObject. This makes the logic used to implement the DDS not very composable. If someone wanted to use the logic from two different DDSes to implement a single one (for example to perform a migration, or to reuse the logic in multiple places like Directory almost does with map)) it is difficult. This approach (Subclassing SharedObject) can also make targeted testing more difficult (for example requiring more mocks), since SharedObject has a significant amount of coupling to runtime types. Further more, the existing pattern has more boilerplate than necessary as the existing pattern of implementing IChannelFactory for each SharedObject is repetitive. Additionally, this subclassing based pattern tends to lend itself toward exporting classes, which lead to the use of both SharedObject and the factory classes in exported APIs which still impacts legacy+alpha API surfaces leaking all the base classes into the API surface. This change introduces a new pattern, where a new purely internal interface is implemented, `SharedKernelFactory`, and passed to `makeSharedObjectKind` which handles everything else. The `SharedKernelFactory` exposes the impelementation of the SharedObject policy as the new `SharedKernel` interface. This object is separate from the app facing view, making it easier to expose minimal APIs (Like SharedTree's ITree) without leaking in APIs from SharedObject. While currently this pattern can't support an app facing API with a property which would collide with a property of SharedObject, a limitation of all current SharedObjects, it sets up a approach which could decouple the app facing APi from SharedObject entirely in the future if desired, both making changes to SharedObject and to the app facing API surfaces easier. In addition to just being a cleaner interface to code SharedObjects against which enable future flexibility, it also is key in enabling the migration approach taken by #23729 as it provides a common abstraction that supported SharedObjects can implement (SharedKernel) that is possible to migrate while preserving the object identity of the SharedObject which wraps it; This scenario is the main motivation for doing this now. The approach of splitting out the core implementation logic for a DDS was inspired by SharedMap which did this with its SharedMapKernel. This didn't end up getting used to implement SharedDirectory, but the approach seems effective and could have facilatated such use. This pattern has been successfully applied to tree and is fully hooked up to tree in this change. This work is split out from #23729 and has some forward looking features to enable supporting SharedMap, like `thisWrap` which is required to implement SharedMap's "set" function which returns `this`.
…to migrationSHim
…to migrationSHim
…to migrationSHim
…to migrationSHim
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Proof of concept Work in Progress.
Introduce "SharedObjectKernel" and "SharedKernelFactory" abstraction as internal types to use to build SharedObjects.
These abstractions separate the DDS specific policy logic from the state and actual SharedObject instance, as well as avoid using subclassesing as an API to reduce coupling.
For compatibility with the existing runtime logic using SharedObjects, a generic SharedObjectFromKernelFull implementation is included which wraps the kernel based policies, though the API to use it is just the makeSharedObjectKind function, avoiding leaking the class even as an internal API.
SharedObjectFromKernelFulluses a proxy to graft the user facing API surface onto the same object that the runtime talks to (which provides op application and such). Ideally these would remain separate objects to avoid exposing the APIs the runtime uses to customers. If all DDSs used this implementation, modifying the runtime to provide this separation would be practical (user facing API could be behind a property on the SharedObject of the runtime facing one could be behind a symbol or map from the user facing one)This approach is intended to be as similar as possible the migration adapter will have a single shared object that can delegate to a kernel which gets replaces when the migration occurs.
Breaking Changes
This POC currently breaks some legacy APIs and tests.
Reviewer Guidance
The review process is outlined on this wiki page.
This code is not ready for review, however feedback on the high level approach of using kernels and the use of the proxy would be helpful.
It's possible to avoid the proxy leaving several options for how to setup the APIs:
return thisto avoid any behavioral changes for now.Option 4 seems like the best choice: feedback on if that is practical and how to do it would be useful.