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

Integrate OpProcessingController functionality into LoaderContainerTracker #5635

Merged
merged 8 commits into from
Mar 25, 2021

Conversation

curtisman
Copy link
Member

@curtisman curtisman commented Mar 23, 2021

Fully replace usage of OpProcessingController with LoaderContainerTracker

  • Container created by Loader added to it will be automatically tracked, removing explicity addDeltaManager
  • OpProcessingController manually track op outbound and inbound to keep track of inflight ops. LoaderContainerTracker do less tracking, instead it use the follow condition to tell if the containers are in sync
    • No isDirty containers
    • No extra clientId in quorum of any container that is not tracked and still opened.
      • i.e. no pending Join/Leave message.
    • No unresolved proposal (minSeqNum >= lastProposalSeqNum) [New inLoaderContainerTracker]
    • lastSequenceNumber of all container is the same
    • clientSequenceNumberObserved is the same as clientSequenceNumber sent
      • This overlaps with !isDirty, but include task scheduler ops that isDirty doesn't cover
      • Trailing NoOp is tracked and don't count as pending ops. See Issue Reconsider noop coalescing #5629

This fixes pendingOps.spec.ts test where there is a bad interaction between OpProcessingController and LoaderContainerTracker where OpProcessingController paused the DeltaManager while LoaderContainerTracker is waiting.

OpProcessingController is mark as deprecated and will be remove on the next release to allow a grace period for migration

Also:

  • Using the debug package to trace ops and wait for debugging (set env DEBUG to fluid:test-utils:ops or fluid:test-utils:wait)
  • Fixes Asserts in leader end-to-end tests do not fail tests #5572: listener with assert should be unregister after test in leader.spec.ts, as those condition would not be true .
  • Convert more test to create and load containers with TestObjectProvider to get automatic tracking
  • Quorum Proposal should throw a Error instead of a string when disconnected.

}

const minSeqNum = containersToApply[0].deltaManager.minimumSequenceNumber;
Copy link
Contributor

@anthony-murphy anthony-murphy Mar 24, 2021

Choose a reason for hiding this comment

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

it's a bit weird we need to special case the quorum, won't other consensus structures have the same issue? i'd rather make the test wait for the side effect it cares about, approve or reject in this case

Copy link
Member Author

Choose a reason for hiding this comment

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

All the consensus data structure only need the round trip. The quorum proposal is unique in that it allow other client to reject, thus needs to wait until minSeqNumber to catch up.

It is debatable that whether we should have the test wait for proposal accepted or not and just let op resume in the mean time. But I do see a benefit to basically ensure the system has completely "settled".

@github-actions github-actions bot requested a review from anthony-murphy March 25, 2021 02:26
@curtisman curtisman merged commit 4006ac8 into microsoft:main Mar 25, 2021
@curtisman curtisman deleted the control branch March 25, 2021 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asserts in leader end-to-end tests do not fail tests
2 participants