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: odp segment manager #402

Merged
merged 10 commits into from
Aug 26, 2022
Merged

feat: odp segment manager #402

merged 10 commits into from
Aug 26, 2022

Conversation

Mat001
Copy link
Contributor

@Mat001 Mat001 commented Aug 23, 2022

Summary

  • ODP Segment manager for Python-sdk
  • Provides an internal api for fetching segments with given apiKey, apiHost, customer key, customer value, segments subset and options.
  • See acceptance criteria in the ticket OASIS-8405 below.

Test plan

  • added unit tests
  • later FSC tests

Issues

@Mat001 Mat001 self-assigned this Aug 23, 2022
@Mat001 Mat001 requested a review from a team as a code owner August 23, 2022 07:49
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A few changes suggested.

optimizely/helpers/enums.py Outdated Show resolved Hide resolved
optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
tests/test_odp_segment_manager.py Outdated Show resolved Hide resolved
tests/test_odp_segment_manager.py Show resolved Hide resolved
@Mat001
Copy link
Contributor Author

Mat001 commented Aug 24, 2022

@jaeopt Fixed your comments. Note that I will need to wait for Andy to merge OdpEventManager and then I will add couple of things to this PR -> refactor some tests and updated odp event. You can have another look now if you like, but I will ask to review again after I'm done with that change.

@Mat001 Mat001 requested a review from jaeopt August 24, 2022 06:38
@Mat001
Copy link
Contributor Author

Mat001 commented Aug 24, 2022

@andrewleap-optimizely Got your changes in as well. Feel free to give feedback or wait until I make final changes to a few tests and odp event after the OdpEventManager merge.

@andrewleap-optimizely
Copy link
Contributor

...wait until I make final changes to a few tests and odp event after the OdpEventManager merge.

The suggested test changes blocked by OdpEventManager I was referring to are for ZaiusRestApiManager 😃.

Copy link
Contributor

@andrewleap-optimizely andrewleap-optimizely left a comment

Choose a reason for hiding this comment

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

Looks really good! Just a couple little things

tests/test_odp_segment_manager.py Show resolved Hide resolved
optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewleap-optimizely andrewleap-optimizely left a comment

Choose a reason for hiding this comment

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

Just a few more minor suggestions

optimizely/odp/odp_segment_manager.py Outdated Show resolved Hide resolved
tests/test_odp_segment_manager.py Outdated Show resolved Hide resolved
tests/test_odp_segment_manager.py Outdated Show resolved Hide resolved
self.logger.debug('ODP cache hit. Returning segments from cache.')
return segments

self.logger.debug('ODP cache miss. Making a call to ODP server.')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessarily a cache miss... We might want to split this into two debug logs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewleap-optimizely Not sure what you mean, can you clarify? If we don't hit the cache we miss it no?
U mean if we don't hit the cache there is the third option that is not a cache miss?

Rb segment manager has the same btw

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that the cache might not have been checked, because ignore_cache was set.

Yea I should change it in ruby and probably Java where I pulled it from 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this:

if not ignore_cache and not reset_cache:
    segments = self.segments_cache.lookup(cache_key)
    if segments:
        self.logger.debug('ODP cache hit. Returning segments from cache.')
        return segments
    self.logger.debug('ODP cache miss.')

self.logger.debug('Making a call to ODP server.')

Otherwise looking at the logs with a disabled cache may confuse the reader into thinking the cache is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, yeah, that makes total sense. good catch

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM with a nit. Can you remove "WIP" from the PR title?

optimizely/odp/odp_options.py Outdated Show resolved Hide resolved
@Mat001 Mat001 changed the title WIP: Mpirnovar/odp segment manager Mpirnovar/odp segment manager Aug 25, 2022
@Mat001 Mat001 changed the title Mpirnovar/odp segment manager feat: odp segment manager Aug 26, 2022
Copy link
Contributor

@andrewleap-optimizely andrewleap-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

@Mat001 Mat001 merged commit 415a666 into master Aug 26, 2022
@Mat001 Mat001 deleted the mpirnovar/odp_segment_manager branch August 26, 2022 19:28
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