[GC] Make getAttachGCData required on IFluidDataStoreChannel and make implementation consistent #21347
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
… the logic consistent
agarwal-navin
requested review from
a team,
pragya91,
markfields,
jatgarg,
tyler-cai-microsoft,
kian-thompson and
rajatch-ff
and removed request for
a team
github-actions
bot
added
area: runtime
⯅ @fluid-example/bundle-size-tests: +1.79 KB
Baseline commit: 15a26ae |
markfields
reviewed
Jun 10, 2024
| .filter( | ||
| ([key, _]) => | ||
| // Take GC data of bounded data stores only. | ||
| !this.contexts.isNotBound(key), |
There was a problem hiding this comment.
tl;dr - I would strongly encourage you to use the visitor pattern (similar example) to share the structure of this do-while loop between both functions here.
Reasons/Thoughts:
- The logic should be the same, but people won't necessarily know to update both places
- To that point... you are already diverging the logic on this line. Shouldn't it check the same 3 conditions as above?
- Are there other places the logic differs, and can we keep it the same so we can share the code?
There was a problem hiding this comment.
I made a change for this but it looks slightly risky. Let me do this in a follow-up PR.
markfields
reviewed
Jun 10, 2024
| } while (notBoundContextsLength !== this.contexts.notBoundLength()); | ||
|
|
||
| // Get the outbound routes (aliased data stores) and add a GC node for this channel. | ||
| builder.addNode("/", Array.from(this.aliasedDataStores)); |
There was a problem hiding this comment.
While you're here, do we have a way to get the absolute path of this channel collection? For nested data stores, it won't be "/".
Fine to track as a follow-up, if someone would just tell us where!
There was a problem hiding this comment.
The best way would be to create a handle for this channel and use it's path. Agreed on a follow-up.
markfields
reviewed
Jun 10, 2024
| @@ -526,9 +528,14 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable { | |||
|
|
|||
| /** Package up the context's attach summary etc into an IAttachMessage */ | |||
| private generateAttachMessage(localContext: IFluidDataStoreContextInternal): IAttachMessage { | |||
| const { attachSummary } = localContext.getAttachData(/* includeGCData: */ true); | |||
| // Get the attach summary. | |||
| const attachSummary = localContext.getAttachSummary(); | |||
| const type = localContext.packagePath[localContext.packagePath.length - 1]; | |||
There was a problem hiding this comment.
oh funny, this code could have got it from the getAttachData call but didn't.
Having callers do that instead of returning it is good, it frees up this change. I think I never considered that!
markfields
reviewed
Jun 10, 2024
| @@ -244,6 +241,8 @@ describe("Data Store Context Tests", () => { | |||
| dataStoreAttributes.isRootDataStore, | |||
| "Local DataStore root state does not match", | |||
| ); | |||
| const type = | |||
There was a problem hiding this comment.
Probably worth adding a free function somewhere to do this, it's worth naming and codifying in one place IMO.
markfields
approved these changes
Jun 10, 2024
agarwal-navin
added a commit
that referenced
this pull request
Jun 14, 2024
…sitor pattern (#21379) Follow up from this comment - #21347 (comment) During summarization or attachments, both GC and summarize runs and they iterate over data stores and channel contexts during summarization. The logic to filter and validate is duplicated for both which makes it prone to get diverged. This PR updates the following set of function to use the [visitor](https://en.wikipedia.org/wiki/Visitor_pattern) pattern to iterate over data stores / channel contexts: - `summarize` and `getGCData`. - `getAttachSummary` and `getAttachGCData`. One logic change in this PR: - It updates `ChannelCollection::getAttachSummary` to use a visisted set to ensure that a node is only summarized oncen similar to `FluidDataStoreRuntime::getAttachSummary`. It earlier used the summary tree to do this. #8257
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.
getAttachGCDatawas added toIFluidDataStoreChannelin 2.0.0-rc.2.0 4 months ago via #19275. This PR does the following:getAttachGCDatarequired onIFluidDataStoreChannel.getAttachGCDatalogic consistent across channel collection, data store context, data store runtime and channel contexts. I found it confusing to follow the existing flow due to the implementation being different for various layers. This is how it works now:ChannelCollectionandFluidDataStoreRuntimeboth implementsgetAttachGCDatawhich iterates through all it's children and gets their GC data by callinggetAttachGCData.FluidDataStoreContextandLocalChannelContextboth implementgetAttachGCDatawhich returns the GC data for this node.getAttachSummaryandgetAttachGCDatainChannelCollection:generateAttachMessage.AB#8199