HybridContainer #25643
HybridContainer #25643
Conversation
github-actions
bot
added
area: server
There was a problem hiding this comment.
Pull Request Overview
This PR implements a hybrid container implementation for the OSS side as a private feature for FRS, with extensive refactoring to enable customization. The PR adds support for ephemeral containers, GitManager configuration decorators, and introduces async context for parameter passing.
Key changes include:
- Enhanced GitManager creation with configurable decorators and security filtering
- Scribe lambda abstraction for FRS customization with document validation
- Document creation flow improvements with staging traces and validation
Reviewed Changes
Copilot reviewed 44 out of 46 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/src/tenant.ts | Enhanced GitManager creation with configurable decorators and extensive documentation |
| services/src/documentManager.ts | Refactored duplicate code into shared method for document static properties |
| services-utils/src/asyncContext.ts | Added generic return type support for bindContext method |
| services-telemetry/* | Added new telemetry properties and events for hybrid container tracking |
| services-shared/src/storage.ts | Major refactoring with staging traces, validation, and extensibility points |
| services-shared/src/socketIoServer.ts | Fixed broken markdown links in documentation |
| services-core/src/trace.ts | Added StageTrace utility class for performance tracking |
| services-client/src/storage.ts | Added comprehensive GitManager configuration decorator system |
| lambdas/src/scribe/* | Abstracted summary writer creation and added document validation |
| historian/* | Added post-ephemeral container checker interface and query parameter support |
| gitrest/* | Enhanced summary creation with consistent commit timestamps |
Files not reviewed (2)
- server/gitrest/pnpm-lock.yaml: Language not supported
- server/historian/pnpm-lock.yaml: Language not supported
Co-authored-by: Copilot <[email protected]>
server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts
Show resolved
Hide resolved
|
|
||
| let postIsEphemeral: boolean = isEphemeral; | ||
| if (postEphemeralContainerChecker !== undefined) { | ||
| postIsEphemeral = await postEphemeralContainerChecker.postEphemeralContainerCheck( |
There was a problem hiding this comment.
I think the naming here is unclear. We seemingly already know that the post is ephemeral (see above). Why do we need to do another check for ephemeral? Is this to actually check if it's Hybrid, or is the above information unreliable?
There was a problem hiding this comment.
Yes, this is a post check to see if a container is hybrid or not, and have following rules in FRS:
- If this is empheral container treat it as empheral container
- If it is the consistent flush for initial summary upload, treat it as dc container
- If it is hybrid container but sync has not happen yet, all summary read should treat as empheral container
| * @returns A boolean indicating whether the document is valid. | ||
| */ | ||
| protected async isDocumentValid(isEphemeralContainer: boolean | undefined): Promise<boolean> { | ||
| return true; |
There was a problem hiding this comment.
I think a comment here would be helpful, otherwise this looks a bit silly without context. Something like:
// This is a placeholder method and can be overwritten by extending this class
// and customizing the SummaryWriterFactory.| * | ||
| * @internal | ||
| */ | ||
| export const GitManagerConfigDecorators = { |
There was a problem hiding this comment.
It seems like all of this security stuff could be pulled into a separate PR? I can't see how this is related to Hybrid Containers, and IMO requires its own separate review.
There was a problem hiding this comment.
This is also a placeholder for FRS extention. In frs, when calling getGitManager I give some of this decorators to decorate headers and query strings for http call to historian
| return this.traces; | ||
| } | ||
| public stampStage(stage: T): void { | ||
| const now = performance.now(); |
There was a problem hiding this comment.
I don't think this will function as expected. performance.now() is not the same as Date.now(). It returns a timestamp based on the process start time as 0, so it is not helpful for tracing information across potentially multiple processes, since each process will have an arbitrary start time, making these trace timestamps completely uncorrelated.
Performance.now()
There was a problem hiding this comment.
I think I just moved the trace class from nexus to shared location so I can use in other flow, create doc flow in this case. But I will look if performance.now() did anything funny now
7 participants
Description
This is hybrid container implementation for OSS side. I am planning to make hybrid container as private feature for FRS only as of now and make it available later when we can prove things are working fine
Breaking Changes
None, quite bunch of refactor and enrichment so FRS could provide some customization
Reviewer Guidance
IPostEphemeralContainerCheckerto be able to get behaviors after EC param check for Hybrid container.StageTracefrom nexus to a shared library so create doc could consume, which will be benefit for our future ICM invisitagation