8000 Fix: ODP Event manager not consistent in triggering events. by mnoman09 · Pull Request #326 · optimizely/csharp-sdk · GitHub
[go: up one dir, main page]

Skip to content

Fix: ODP Event manager not consistent in triggering events. #326

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&rdqu 8000 o;, 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

Merged

Conversation

mnoman09
Copy link
Contributor
@mnoman09 mnoman09 commented Feb 8, 2023

Summary

  • 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.

Test plan

Passing build link with FSC: link

Issues

  • [TODO]

@mnoman09 mnoman09 closed this Feb 8, 2023
@mnoman09 mnoman09 reopened this Feb 9, 2023
@msohailhussain msohailhussain changed the base branch from master to mike/odp-usercontext-optimizelyclient February 9, 2023 20:40
@mnoman09 mnoman09 changed the base branch from mike/odp-usercontext-optimizelyclient to master February 10, 2023 11:09
@mnoman09 mnoman09 closed this Feb 10, 2023
@mnoman09 mnoman09 reopened this Feb 10, 2023
@mnoman09 mnoman09 changed the base branch from master to mike/odp-usercontext-optimizelyclient February 13, 2023 15:02
@mnoman09 mnoman09 changed the title Testing (DO NOT REVIEW) Fix: ODP Event manager not consistent in triggering events. Feb 13, 2023
@mnoman09 mnoman09 removed their assignment Feb 13, 2023
Copy link
Contributor
@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mnoman09 mnoman09 merged commit 99a42d8 into mike/odp-usercontext-optimizelyclient Feb 13, 2023
@mnoman09 mnoman09 deleted the mnoman/odpEventFlushFix branch February 13, 2023 19:20
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0