Fix: ODP Event manager not consistent in triggering events. #326
Conversation
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
…EventManager should not flush.
…concurrentDictionary" This reverts commit 64cdf80.
msohailhussain
pushed a commit
that referenced
this pull request
Feb 14, 2023
) * Include ODP Manager instantiation in Optimizely.cs Includes lint/format updates...Sorry PR-Reviewer :-/ * Add FetchQualifiedSegments to Optimizely.cs * Add FetchQualifiedSegments to UserContext * Add IdentifyUser, fetch segments callback, & docs * WIP existing test fixes * Fix legacy test (mock) constructors * Add SendOdpEvent to Optimizely * WIP Init OdpManager * WIP Add OptimizelySdkSettings & fix some legacy tests * WIP fixes * Remove early UpdateOdpSettings * Add internal accessor for ProjectConfig Segments Sorry for the linter formatting updates * Linter fixes * Preprocessing conditional compilation directives * Add Segments array to ProjectConfig * Lint corrections * More linter fixes hopefully ;-) * WIP PR review changes * Lint fix * Lint fix * Add async FetchQualifiedSegments * WIP parse TypedAudience to ODP Segments * WIP parsing Segments * More ODP test datafile more scenarios * Revert IMultipleConditions concept * Correct Segments from datafile * Lint fix * Add segment unit tests * WIP satisfying legacy tests * Remove Ignore from legacy tests * Finalize SetupOdp; add Dispose to OptimizelyUserContext * Add missing preprocessing conditions * Fix linting * PR code review updates * Lint fixes * feat: Fix NotificationCenter Issue for ODPManager (#324) * PR changes * More PR changes * proposed changes * Understanding & additional updates * WIP Refactors & test fixes * WIP correcting tests... * Refactors * WIP OdpEventManagerTest resolutions * Remove NotificationCenter from NotificationCenterRegistry on Dispose * Fix Disposed = true location * Modify NCR Dispose * Mods to NCR and NC disposal * Mods to NCR and NC disposal * Fix OdpEventManager & tests * Remove double lock in NotificationCenterRegistry * Fix final test corrections * Pass logger into NotificationCenterRegistry * Last PR change request * Remove TODO comments Co-authored-by: msohailhussain <sohail.mirza@SMirza-MBP.local> * Lint fixes + Copyright year updates * Lint fix PollingProjectConfigManager * Trying to fix TestPollingConfigManagerBlocksWhenProjectConfigIsNotProvided * Another attempt to figure out TestPollingConfigManagerBlocksWhenProjectConfigIsNotProvided * Update OptimizelySDK/Config/PollingProjectConfigManager.cs Co-authored-by: Muhammad Noman <Muhammadnoman@folio3.com> * Update OptimizelySDK/Config/HttpProjectConfigManager.cs Co-authored-by: Muhammad Noman <Muhammadnoman@folio3.com> * Update OptimizelySDK/Config/FallbackProjectConfigManager.cs Co-authored-by: Muhammad Noman <Muhammadnoman@folio3.com> * Update OptimizelySDK/Config/FallbackProjectConfigManager.cs Co-authored-by: Muhammad Noman <Muhammadnoman@folio3.com> * Implement a NoOpOdpManager * Remove configurable batch size for OdpEventManager * Fix lint (Mike not happy) * OMG Linter needs config * Split condition for UpdateSettings & NotificationCenterRegistry add * Remove optional chaining * PR review changes * Fix InternalsVisibleTo & use internal for testing * PR change requests * Add unit test for defaults from OdpManager & below * Remove NoOpOdpManager & default OdpManager instantiation * Lint fixes * Allow signed + assigned friend assemblies * Use pubkey of OptimizelySDK.Tests * Testing InternalsVisibleTo in csproj * More testing signed assems * WIP fix InternalsVisibleTo with Release build * Require providing SDK key in HttpProjectConfigManager constructor * Fix: Some suggested changes related to ODP (#325) * Some suggested changes to fix null pointer exception * - Removed batchsize support from ODP event manager - Made some general fixes * Flush previous event * reverting this change Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com> * Merge fixes + lint * Lint fixes * Build fix and lint * PR change requests * Lint fix * Fix: ODP Event manager not consistent in triggering events. (#326) * Refactored OdpEventManager flushEvent function. * To wait for events using flushInterval when eventBatch is not empty. * Or wait indefinitely if no event is in batch until any event arrive. * Avoid calling flush on shutdown if no events are present * Avoid calling unnecessary flushQueue if _currentBatch.Count is zero. * Fixed the test as when there are no remaining items in queue then odpEventManager should not flush. * Removing mutex lock from notification registry instead using concurrentDictionary * Revert "Removing mutex lock from notification registry instead using concurrentDictionary" This reverts commit 64cdf80. * Added Thread.sleep which somehow enabling thread to shutdown gracefully. --------- Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com> --------- Co-authored-by: msohailhussain <sohail.mirza@SMirza-MBP.local> Co-authored-by: Muhammad Noman <Muhammadnoman@folio3.com> Co-authored-by: mnoman09 <m.nomanshoaib09@gmail.com>
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
Test plan
Passing build link with FSC: link
Issues