feat(map): add opt-in sort-key iteration to SharedDirectory#27145
Draft
tylerbutler wants to merge 9 commits intomicrosoft:mainfrom
Draft
feat(map): add opt-in sort-key iteration to SharedDirectory#27145tylerbutler wants to merge 9 commits intomicrosoft:mainfrom
tylerbutler wants to merge 9 commits intomicrosoft:mainfrom
Conversation
Adds an opt-in `detect_changes` job to the client build pipeline that
computes the merge-base vs the PR's target branch and emits a pnpm
`--filter "...[<sha>]"` expression as an output variable. Downstream
test jobs consume it via `npm_config_filter`, so pnpm natively scopes
recursive runs to packages changed in the PR (plus their transitive
dependents) — no wrapper scripts, no package.json changes.
Activates when one of:
* `enableChangedPackageTestScoping` pipeline parameter is true, OR
* the PR source branch name contains `test/filtered-ci/` — a
branch-name convention that lets contributors exercise the feature
from an auto-triggered PR build without flipping parameters.
When the detect job is skipped (non-PR builds, or PR builds that
haven't opted in) its output variables resolve to empty strings, which
downstream jobs interpret as "no filter — run every package" — so
pipeline behavior is effectively byte-identical to today for non-opt-in
runs.
The merge-base SHA is passed to pnpm's selector instead of a branch ref
because pnpm's `[ref]` uses a two-dot diff (pnpm/pnpm#9907), which
would pick up unrelated commits from the target branch as "changed".
🤖 Generated with [Nori](https://noriagentic.com)
Co-Authored-By: Nori <contact@tilework.tech>
Addresses review feedback on the scoping PR: - Skip Coverage_tests and Test_* jobs entirely (not just their test steps) when detect_changes reports shouldRunTests=false. Empty (detect_changes skipped) still runs, preserving the historical non-opt-in behavior. Saves agent allocation + checkout + install for no-package-change PRs. - Remove the now-redundant step-level Start Test condition and orphaned `shouldRunTests` job variable; `scopedPnpmFilter` is still exposed as a job variable because test-task steps consume it. - Shallow-clone the detect_changes checkout (fetchDepth: 200) with a runtime unshallow fallback guarded by .git/shallow presence, so full clones don't error on --unshallow. - Cross-reference `pr: paths: include:` in build-client.yml from FULL_RUN_PATTERNS to flag the convention-based overlap. - Condense duplicated "historical behavior" narration across the diff. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
PR microsoft#27134 revealed that `succeeded()` on `Coverage_tests` and `Test_*` false-skips the job when `detect_changes` is Skipped, contrary to the documented "skipped deps count as successful" behavior. Check `dependencies.build.result` directly so the test jobs run for non-opt-in PRs (where `detect_changes` is intentionally skipped) while still respecting a 'false' output signal when scoping is active. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
The parameter only takes effect on manually-queued builds, where an operator can tick it in the ADO UI. Auto-triggered PR builds always run with the `false` default, so the only practical opt-in is the 'test/filtered-ci/' branch-name substring. Removing the parameter eliminates a lever that can't be pulled from a developer's normal workflow and simplifies the detect_changes activation condition. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
…tory Onboarding reference for contributors: covers the pending-state / lifetime model shared by both DDSes, per-DDS internals (kernel split, snapshot formats, tree topology, dispose lifecycle, ordering), wire-op shapes, event suppression rules, and known subtleties. The README stays user-facing; this is the doc you want when you're editing the code. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Adds a replicated, opt-in custom iteration order to SharedDirectory driven by string "sort keys" attached to individual keys and child subdirectories. The default iteration order is unchanged. New API on IDirectory / ISharedDirectory: - setSortKey(key, sortKey | undefined) / setSubDirectorySortKey - keysByOrder / valuesByOrder / entriesByOrder / subdirectoriesByOrder - sortKeyChanged / subDirectorySortKeyChanged (path-carrying) on ISharedDirectoryEvents plus containedSortKeyChanged / containedSubDirectorySortKeyChanged on IDirectoryEvents Ordering semantics: sort-keyed entries first in lexicographic (JS <) order of their sort keys, ties broken by the default iteration order; unkeyed entries follow in default order. Sort keys live alongside their entry's identity — delete(k), clear(), and deleteSubDirectory clear associated sort-key state. Replication: two new op types (setSortKey, setSubDirectorySortKey) wired through the existing pending/sequenced model with reference- identity ack matching. Rollback, reconnect-resubmit, and applyStashedOp paths are extended symmetrically. Snapshot: optional additive fields sortKeys / subdirectorySortKeys on IDirectoryDataObject. The directory snapshot format stays at 0.1; older readers silently ignore the new fields. Plan and test specification at packages/dds/map/SORT-KEYS-PLAN.md. Adding methods to IDirectory is a forward-compat break; declared in typeValidation.broken. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Contributor
|
Hey! You look nice today! Want me to review this PR? Based on the diff (4007 lines, 15 files), I've queued these reviewers:
Toggle checkboxes to adjust, then reply yes to start — or ask me anything! |
Member
Author
|
yes |
Adds six mocha tests exercising edge cases of the SharedDirectory sort-key API that were deferred from the initial feature commit: - T33: rollback of a pending delete restores the key and its sort key. - T34: a local clear while a setSortKey is pending leaves no leftover sequenced sort key after both ops ack. - T42: deleting one subdirectory leaves sibling subdirectorySortKeys entries intact. - T45a/T45b: concurrent delete + setSortKey on the same key from two clients converges; split into two cases covering both sequencing orders, with T45a documenting that a delete-first race leaves the remote setSortKey behaving as pre-registration for a future rebirth of the key (same semantics as T46). - T56: a pending setSortKey whose subdirectory was remotely deleted during a disconnect is dropped silently by the resubmit path when the client reconnects. Also updates SORT-KEYS-PLAN.md to mark these items done in the status header, adjust the test-coverage table, and document the T45 pre-registration clarification. Full @fluidframework/map mocha suite: 973 passing (was 967). 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Covers rollback of setSortKey (T50-T52), detached no-op (T53), and setSubDirectorySortKey mirrors (T54a/b/c) in directory.rollback.spec.ts. T52/T54c exercise pending-entry splice-by-reference-identity via flushSomeMessages(1) + processOneMessage before rollback. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <contact@tilework.tech>
Contributor
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This file contains hidden or 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
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.
Summary
Adds a replicated, opt-in custom iteration order to
SharedDirectorydriven by string "sort keys" attached to individual keys and child subdirectories. The existing default iteration order is unchanged.What's new
API additions on
IDirectory/ISharedDirectory:setSortKey(key, sortKey | undefined)/setSubDirectorySortKey(name, sortKey | undefined)— set or clear (viaundefined) a sort key.keysByOrder()/valuesByOrder()/entriesByOrder()— iterate entries in sort-key order.subdirectoriesByOrder()— iterate child subdirectories in sort-key order.sortKeyChanged/subDirectorySortKeyChanged(path-carrying) onISharedDirectoryEvents;containedSortKeyChanged/containedSubDirectorySortKeyChangedonIDirectoryEvents.Semantics
Sort-keyed entries iterate first, in lexicographic order of the sort key (JS
<, UTF-16 code points). Entries without a sort key follow, in the existing iteration order. Ties on sort key break by the default iteration order — so ordering is deterministic across clients.Sort keys live alongside the entry they annotate:
delete(k)clears the entry's sort key,clear()clears all sort keys in the directory, anddeleteSubDirectory(name)clears the child's sort key on the parent.Replication and back-compat
setSortKey,setSubDirectorySortKey) are wired through the existing pending/sequenced model with reference-identity ack matching. Rollback, reconnect-resubmit, andapplyStashedOppaths are extended symmetrically.sortKeys?/subdirectorySortKeys?onIDirectoryDataObject. The directory snapshot format stays at0.1— older readers silently ignore the new fields.IDirectoryis a forward-compat break; declared inpackage.jsontypeValidation.broken. Back-compat is preserved.Docs and plan
packages/dds/map/SORT-KEYS-PLAN.md(includes implementation status, deviations from the original design, and a resume-in-fresh-session guide).packages/dds/map/ARCHITECTURE.md§9.4 + §11.Status
47 of 67 planned tests landed in
packages/dds/map/src/test/mocha/directory.sortKey.spec.ts. 20 follow-on tests (deferred rollback / snapshot round-trip / fuzz, plus a few edge-case spec entries) are enumerated at the top ofSORT-KEYS-PLAN.mdfor a follow-up PR; all their implementation paths are covered by the landed code.Draft — API Council review required (surfaces
@public @legacyand@beta @legacytypes).