Repository navigation
feat: Expose server-side flag change notifier on Client - #632
Draft
kinyoklion wants to merge 2 commits into
Draft
kinyoklion wants to merge 2 commits into
kinyoklion wants to merge 2 commits into
Conversation
…pendencies Co-Authored-By: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Co-Authored-By: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Contributor
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
This branch has not been deployed
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
The server SDK already had a dependency-aware
ChangeNotifier, but applications had no way to reach it. This PR exposes it onClient, addsflag-change-listenerssupport to the contract test service, and fixes a dependency tracking bug that the new listener tests in sdk-test-harness#514 turned up.Public API (server SDK only). Adds a new pure virtual to
IClient, so any out-of-tree implementation ofIClientmust now implement it:IDataSystemgainsFlagNotifier().BackgroundSyncandFDv2DataSystemreturn their existingChangeNotifier.OfflineSystemandLazyLoadreturn a newNullChangeNotifier, which accepts handlers but never calls them.Fix:
DependencyTrackernever updated existing edges.DependencyMap::Getreturns a copy andDependencyMap::Setusedemplace, which doesn't overwrite. As a result:Setnow usesinsert_or_assign, andUpdateDependencieswrites its changes back. The removal path also had the wrongDataKind: it removed(kind_of_dep, key)instead of(kind, key). Unit tests cover all three cases.Contract tests.
flag-change-listenerscapability and theregisterFlagChangeListenerandunregisterListenercommands.ClientEntitykeeps oneIConnectionper listener ID. The connections are declared afterclient_, so they disconnect before the client is destroyed.ContractTestHook::PostCallbackmoved intoPostJsonCallbackso both callers share it.Verification.
-run 'flag change listeners': 16/16 pass. Before the tracker fix, 14/16 passed; the failures were "a dependency added by an update is followed afterwards" and "a dependency removed by an update is not followed afterwards".gtest_launchdarkly-cpp-server: 574/574 pass.Not in scope:
flag-value-change-listeners. The server SDK has no value change listener API, so the harness skips those 16 tests.Link to Devin session: https://app.devin.ai/sessions/3c3618a040a544a895800896137ad1ca
Open in Devin Desktop: https://app.devin.ai/desktop/session/3c3618a040a544a895800896137ad1ca?variant=devin
Requested by: @kinyoklion
SDK-3355