Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified ScheduleManager and renamed it to InboundBatchAggregator #23446

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

agarwal-navin
Copy link
Contributor

@agarwal-navin agarwal-navin commented Jan 3, 2025

Renamed ScheduleManagerCore to InboundBatchAggregator because its primary objective is to ensure that an incoming batch is fully available in the delta queue before it starts processing.

Also, simplfied it as following:

  • Removed ScheduleManager because all it was doing was calling DeltaScheduler and emitting batchBegin and batchEnd events. Moved this directly into container runtime which was recently refactored to consolidate this logic in one place.
  • Removed the logic to modify batch metadata in delta manager's prepareSend event handler. This is an artifact of very old logic where adding the batch metadata was distributed between container runtime and delta manager. For a long time now, container runtime is in full control of adding (or not adding) batch metadata to messages in a batch. This was done by Move batch processing logic from loader to container runtime #11832.
  • Removed the scheduleManager.spec.ts unit tests. All it was doing was validating batchBegin and batchEnd events which is the same as the ones done in batching.spec.ts unit tests. Moreover, it was using custom logic to identify batch beginning and end from the inbound messages so it wasn't really testing any real code.

 
AB#1981

@Copilot Copilot bot review requested due to automatic review settings January 3, 2025 19:44
@github-actions github-actions bot added area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Jan 3, 2025
);
this.inboundBatchAggregator.setupListeners();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed because the logic inside it can be done in the constructor of InboundBatchAggregator. However, it doesn't have any public members so the alternative would be to not have a member in container runtime for it and do something like void new InboundBatchAggregator ..." similar to how ScheduleManagerCore` was used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably suggest moving the setupListeners content into the constructor as you mention, otherwise the constructor returns a kind-of-wrong object.

InboundBatchAggregator should really probably have a public dispose() that unregisters those listeners which would be the reason for retaining a reference rather than voiding it (to call this.inboundBatchAggregator.dispose() when the ContainerRuntime disposes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I added dispose.

@@ -82,55 +40,29 @@ class ScheduleManagerCore {
private readonly getClientId: () => string | undefined,
private readonly logger: ITelemetryLoggerExt,
) {
// Listen for delta manager sends and add batch metadata to messages
this.deltaManager.on("prepareSend", (messages: IDocumentMessage[]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this logic is not needed any more because container runtime handles this before it sends the batch to delta manager. See PR description for more details.

Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it - would suggest moving the event setup stuff into the constructor though.

If you don't pursue a dispose() in this PR and stick with the void, maybe just open a task/todo.

* 2. It creates instance of ScheduleManagerCore that ensures we never start processing ops from batch
* unless all ops of the batch are in.
* This class ensures that we aggregate a complete batch of incoming ops before processing them. It basically ensures
* that we never start processing ops in ab batch IF we do not have all ops in the batch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* that we never start processing ops in ab batch IF we do not have all ops in the batch.
* that we never start processing ops in a batch IF we do not have all ops in the batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants