Repository navigation
Conversation
Spec for pooling SchemaClientBoundImpl per (name, vendor, version) schema identity instead of one private cache per datastore. See docs/adr/0005-shared-refcounted-schema-bound-registry.md.
.scratch/open/11-schema-bound-registry-spec.md
Implements the schema-bound registry per .scratch/open/11-schema-bound-registry-spec.md and ADR 0005. - Add pkg/datastore/clients/schema/registry.go: a process-wide Registry that pools *SchemaClientBoundImpl instances keyed by (name, vendor, version), refcounted per datastore via GetOrCreate/Handle.Close(). The pooled instance's internals (path-keyed lookup cache, per-entry coalescing) are untouched; only construction/lifecycle move from one-per-datastore to one-per-schema-identity. Release on an unknown or already-zero-refcount triple, and repeated Handle.Close(), are no-ops. - Wire the registry into Datastore.New/Delete (pkg/datastore/datastore_rpc.go): New acquires a handle via registry.GetOrCreate and stores it with its concrete *schemaClient.Handle type so Close is directly callable without a type assertion; Delete calls Close; Stop does not (shutdown and explicit deletion remain distinct teardown paths). The handle is also closed if tree construction fails partway through New. - Construct the registry once in pkg/server/schema.go alongside schema.Client construction (local or remote) and thread it into both datastore.New call sites (pkg/server/datastore.go, pkg/server/server.go) in place of the raw schema.Client. - Remove remoteClient's ttlcache and the RemoteSchemaCache config it was driven by (pkg/schema/remote.go, pkg/config/config.go, pkg/config/defaults.go): the registry is now the single schema-caching mechanism in the process. NewRemoteClient drops the cacheConfig param. go.mod: jellydator/ttlcache/v3 is now an indirect dependency. - SchemaClientBound is unchanged (no Close() added); NewSchemaClientBound remains directly usable by existing test call sites that bypass the registry. Tests: new registry_test.go covers the primary seam per the spec's Testing Decisions (shared pooled instance for the same triple, distinct instances for different triples, refcount keeps an instance alive until every handle closes, idempotent Close, no-op Release on an unknown triple). Existing pkg/server and pkg/datastore lifecycle tests continue to pass with minor wiring updates (newTestServer now also builds a schemaRegistry).
steiler
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
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
Each datastore used to create its own schema-bound client (
SchemaClientBoundImpl) with a private path-keyed lookup cache. Many datastores on the same YANG model (name, vendor, version) duplicated the same cached entries in memory. This PR pools one bound client per schema triplet, shared by all datastores that use it, and frees it when no datastore still holds a handle.We also remove the remote schema client’s TTL cache (
RemoteSchemaCache/ttlcache) so there is a single caching layer—the pooled bound client—not two.How it works
pkg/datastore/clients/schema/registry.go): map(name, vendor, version) → pooled instance + refcount.GetOrCreatereturns a handle that implementsSchemaClientBoundand addsClose()(not on the interface; existing mocks/tests unchanged).Newacquires a handle from the registry;DeletecallsClose()(Stopdoes not—same shutdown vs delete split as before). FailedNewcloses the handle to avoid refcount leaks.schema.Clientsetup; bothdatastore.Newcall sites use it.schema-server.cacheis gone.SchemaClientBoundImplinternals (path map, coalescing) are unchanged. Per-path invalidation on reload is out of scope (whole triplet evicted when refcount hits zero).Test plan
Close)go test ./...Docs
docs/adr/0005-shared-refcounted-schema-bound-registry.md.scratch/open/11-schema-bound-registry-spec.mdCONTEXT.md(Schema client registry / schema-bound handle)