-
Notifications
You must be signed in to change notification settings - Fork 11
chore: Allow for completion reporting for FDv2SourceResult consumption. #128
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
base: ta/SDK-1633/test-data-source-synchronizer
Are you sure you want to change the base?
chore: Allow for completion reporting for FDv2SourceResult consumption. #128
Conversation
| private final Object queueLock = new Object(); | ||
| private final LinkedList<FDv2SourceResult> queue = new LinkedList<>(); | ||
| private final LinkedList<CompletableFuture<FDv2SourceResult>> pendingFutures = new LinkedList<>(); | ||
| private final IterableAsyncQueue<FDv2SourceResult> resultQueue = new IterableAsyncQueue<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior here ends up not really queuing, but it makes the implementation simple.
| future.complete(FDv2SourceResult.shutdown()); | ||
| } | ||
| return CompletableFuture.anyOf(shutdownFuture, future).thenApply(r -> (FDv2SourceResult) r); | ||
| if (!initialSent.getAndSet(true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the need for the lock, and this just needs to be basically a latch.
| private final LinkedList<CompletableFuture<FDv2SourceResult>> pendingFutures = new LinkedList<>(); | ||
| private final IterableAsyncQueue<FDv2SourceResult> resultQueue = new IterableAsyncQueue<>(); | ||
| private final CompletableFuture<FDv2SourceResult> shutdownFuture = new CompletableFuture<>(); | ||
| private volatile boolean closed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed closed because I don't see a real need for it. It is a bit of an optimization, but this isn't very high frequency, so it seems nice to just be simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be identical to the FDv1 tests, but using the FDv2 version.
|
bugbot review |
08f24e2 to
a26dc8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Note
Medium Risk
Touches core FDv2 data-source result consumption and introduces new callback/lifecycle semantics that could affect threading or resource cleanup if misused; behavior is covered with new integration tests but remains concurrency-sensitive.
Overview
Adds consumption completion reporting for FDv2 results by making
FDv2SourceResultCloseable, adding an optional completion callback pluswithCompletion(), and updatingFDv2DataSourceto use try-with-resources when handling initializer/synchronizer results so callbacks reliably fire.Refactors
TestDataV2to use this mechanism (wrapping results with per-synchronizer completions, switching its internal queueing toIterableAsyncQueue, and removing the old polling-basedawaitPropagationhelper), and addsTestDataV2WithClientTestto assert initialization, flag updates/deletes, rule/target behavior, status updates, and multi-client propagation.Written by Cursor Bugbot for commit 08f24e2. This will update automatically on new commits. Configure here.