Skip to content
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

feat: Fix NotificationCenter Issue for ODPManager #324

Conversation

mikechu-optimizely
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely commented Jan 6, 2023

Summary

Test plan

  • Testing with the main PR for branch odp-usercontext-optimizelyclient

Issues

  • FSSDK-8476
  • FSSDK-8771

@@ -63,6 +62,15 @@ public class LruCache<T> : ICache<T>
{
_mutex = new object();

// TODO: Please add a condition to check minimum value of maxSize as well.
// @msohailhussain What's the minimum value for maxSize?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msohailhussain I'd like to better understand the requirement. DM me when you have a moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Check with @jaeopt

@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review January 13, 2023 19:03
@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner January 13, 2023 19:04
@mikechu-optimizely mikechu-optimizely changed the title PR changes feat: Fix NotificationCenter Issue for ODPManager Jan 13, 2023
NotificationCenter?.SendNotifications(NotificationCenter.NotificationType.
OptimizelyConfigUpdate);

#if USE_ODP
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bring this code first and then notify to user notification.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

@mikechu-optimizely mikechu-optimizely merged commit c68eea9 into mike/odp-usercontext-optimizelyclient Jan 18, 2023
@mikechu-optimizely mikechu-optimizely deleted the mike/odp-usercontext-optimizelyclient-mods branch January 18, 2023 19:38
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 <[email protected]>

* 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  <[email protected]>

* Update OptimizelySDK/Config/HttpProjectConfigManager.cs

Co-authored-by: Muhammad Noman  <[email protected]>

* Update OptimizelySDK/Config/FallbackProjectConfigManager.cs

Co-authored-by: Muhammad Noman  <[email protected]>

* Update OptimizelySDK/Config/FallbackProjectConfigManager.cs

Co-authored-by: Muhammad Noman  <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: msohailhussain <[email protected]>
Co-authored-by: Muhammad Noman  <[email protected]>
Co-authored-by: mnoman09 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants