Second round of request handler changes #3090
Merged
Conversation
This file contains 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
… method is renamed to resolveHandle(). Note: That causes data store runtime to have both request() & resolveHandle(). In next iteration, they will have different behaviours - acecss to DDSs will only happen for handle route, i.e. it would be impossible to request DDS from data store unless data store handle implements such access. This is equivalent to making DDSs private. 'path' is removed from both IFluidHandle & IFluidRouter. Introducing handleFromLegacyUri() for back compat. Uisng it in couple places in this chage to scope amount of churt - all places where it's used needs revisit. Fixing how we refer to task manager ID and how we access task manager & agent sceduler Exposing task manager on IContainerRuntimeBase. Ideally next step should be to pass it through scoping mechanism (and start removing access to "globals" in container) Vltava: changing registry creation pattern - hiding component type names, forcing everything to go through factories. Same pattern should be expanded to all of our examples.
vladsud
requested review from
anthony-murphy,
ChumpChief,
skylerjokiel and
agarwal-navin
|
Ping |
Not related to this PR but we don't need this anymore right? If I am not wrong, ContainerRuntime cannot have ops other than these two. I am going to update this in a separate change that I have planned. Refers to: packages/framework/last-edited-experimental/src/setup.ts:18 in bb3a7a6. [](commit_id = bb3a7a6, deletion_comment = False) |
curtisman
reviewed
Aug 12, 2020
agarwal-navin
approved these changes
Aug 12, 2020
…into resolveHandle # Conflicts: # docs/docs/fluid-handles.md # docs/docs/visual-component.md # docs/tutorials/dice-roller-comments.tsx # docs/tutorials/dice-roller.md # docs/tutorials/dice-roller.ts # docs/tutorials/sudoku.md # examples/data-objects/client-ui-lib/package.json # examples/data-objects/vltava/package.json # examples/data-objects/vltava/src/components/anchor/anchor.ts # examples/data-objects/vltava/src/index.ts # packages/framework/aqueduct/src/data-objects/pureDataObject.ts # packages/framework/last-edited-experimental/src/lastEditedTrackerComponent.ts # packages/runtime/container-runtime/src/containerRuntime.ts
…into resolveHandle # Conflicts: # packages/framework/aqueduct/src/test/defaultRoute.spec.ts # packages/framework/request-handler/src/test/requestHandlers.spec.ts
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.
First, there are a bunch of unrelated changes here, simply because things are broken in master, or implemented improperly where it's hard to go by without fixing them. I may spawn separate PRs for these issues
Key changes:
IFluidHandleContext no longer implements IFluidRouter. It's request() method is renamed to resolveHandle().
This reduces number of responsibilities for both interfaces and makes it clearer on the purpose of interfaces.
Note: This causes DataStoreRuntime to have both request() & resolveHandle().
In next iteration, they will have different behaviors - DDS URIs will fail to resolve when request comes in through request(). I.e. it would be impossible to request DDS from data store unless data store handler implements such access.
This is equivalent to making DDSs private on a data store class.
'path' is removed from both IFluidHandle & IFluidRouter. It has been marked deprecated for many versions.
Introducing handleFromLegacyUri() for back compat. Using it in couple places in this change to scope amount of churn - all places where it's used needs revisit. More on that below.
Fixing how we refer to task manager ID and how we access task manager & agent scheduler. In many places code confuses data store ID vs. type (even though they are the same thing).
Exposing task manager on IContainerRuntimeBase. More on future changes here below.
Vltava: changing registry creation pattern - hiding component type names, forcing everything to go through factories. Same pattern should be expanded to all of our examples, as name we use in registry not always matches type on a factory.
Last edited is simplified - async load path is deprecated, with assumption that it is being used on a critical path of container loading, with detached container creation.
PureDataObject names are busted due to renames for many users (i.e. componentInitializingFirstTime is still used in code in derived classes, when such method was renamed on base class).
Follow up for # 3 & 4 above:
Next step will be to pass data object dependencies to them directly through scoping mechanism on object creation, and start removing access to "globals" in container. The most obvious example why it's bad is PureDataObject.getService() & getMatchMakerContainerService() implementations. Consumers of those are likely not aware that generateContainerServicesRequestHandler() has to be used by container developer. In fact, getMatchMakerContainerService() is not used in our repo (other than UT)!
Better approach is to pass these dependencies through local (to object) scope, and force fluid object implementation to grab and store such dependencies (using handles) within data object itself, for future use. PureDataObjectFactory.instantiateInstance() can validate required dependencies are present. PR #2950 takes a first step on this path.
Basically I want to move to a system where caller passes scope object when creating data objects. And slowly removing access to ContainerRuntime.containerScope (including copy on IDataStoreContext.scope). Similar, remove most abilities to do requests against container for agents inside container.
This is similar paradigm to "regular" coding practices - globals/singletons are bad, they complicate composability of code, leak side-effects and do not allow to "connect" independent pieces of code easily (using adapters). Components should be like classes - clearly expressing their requirements upfront, and consumers satisfying these requirements.