Rework attribution options to be specified by container authors #15397
Conversation
github-actions
bot
added
area: dds
|
There are still some things to resolve with this before it's in a check-in-able state:
(3) and (4) are obviously the biggest things here. |
| ) { | ||
| super(id, dataStoreRuntime, attributes, "fluid_sequence_"); | ||
|
|
||
| this.loadedDeferred.promise.catch((error) => { | ||
| this.logger.sendErrorEvent({ eventName: "SequenceLoadFailed" }, error); | ||
| }); | ||
|
|
||
| let clientOptions = dataStoreRuntime.options; | ||
| if (options?.attribution) { |
There was a problem hiding this comment.
i like the overall pattern, but it would be nice if we could just smash them all together, rather than just propagating attribution.
maybe something like {...dataStoreRuntime.options, ...options} . i just want to be able to add more later and not have to figure why they don't propagate correctly.
| @@ -109,6 +110,10 @@ export interface ISharedSegmentSequenceEvents extends ISharedObjectEvents { | |||
| ); | |||
| } | |||
|
|
|||
| export interface SequenceOptions { | |||
| attribution: IMergeTreeAttributionOptions; | |||
There was a problem hiding this comment.
i wonder if we want to take a firmer stance on what options are, specifically if they can be more than primitive values. probably doesn't need to be solved immediately.
| @@ -24,6 +32,8 @@ export class SharedStringFactory implements IChannelFactory { | |||
| packageVersion: pkgVersion, | |||
| }; | |||
|
|
|||
| public constructor(public readonly options?: SequenceOptions) {} | |||
There was a problem hiding this comment.
kind of related to my above comment, and your inline comment. an idea, that i'm not sure i even like, is to split what you have, options is only primitives, and a services dictionary/object/something. the policy on options would then indicate the key entry of the services to use. not sure how something like this would propagate through the stack. what i do like if it worked out, is applications could potentially centralize, or share a services registry, but have distinct options objects, without needing to compose objects from the two.
There was a problem hiding this comment.
we follow a similar pattern for data store and dds registries, and it works ok.
…to more-authentic-attribution-options
…y specification possible via FluidObject pattern
| @@ -343,6 +340,22 @@ export interface IAttributionCollectionSpec<T> { | |||
| }>; | |||
| } | |||
|
|
|||
| // @public | |||
| export interface IAttributionPolicyFactory { | |||
There was a problem hiding this comment.
still need to do a bit of cleanup of API tags and docs in this realm.
Also not totally convinced I want to expose classes for the policy registry elements or have them be self-named, but that's a bit more minor (self-naming is less of a concern for closed-ecosystem set of options, which is what attribution policies currently are. Maybe we open them up in the future but it's pretty easy to shoot yourself in the foot, so I'd rather not if we can get away with it)
⯅ @fluid-example/bundle-size-tests: +1.34 KB
Baseline commit: 7981852 |
|
This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework! |
Description
This reworks how options affecting attribution get specified--previous method relied heavily on the host setting things up appropriately. Some changes which I think are in the right direction:
Notably, having individualized control at the DDS level controlled by a dataObject's creator seems like the right direction for the future: it's easy to imagine containers which have heterogeneous content involving some SharedStrings that want attribution enabled and other SharedStrings which don't. Making this setting parameterizable via the SharedObject factory satisfies both of those goals. One can even accomplish having the same type of DDS within the same data store with different parameters if enough information about those parameters is persisted in the attributes blob (though that isn't done here, this would require changes to the channel creation flow).
It's probably not ideal that the container author also needs to enable it at the runtime level, but not a huge deal. I also don't feel strongly that that setting should be injected via scope, but since this was already the case for other content, it didn't feel unreasonable.