feat(segment-manager): Adds implementation for ODPSegmentManager #353

yasirfolio3 · 2022-11-07T07:56:09Z

Summary

This PR adds support for ODPSegmentManager.

Test plan

Unit tests added

Issues

FSSDK-8511

2. Suggested changes made.

…odp-segement-apimanager

jaeopt

Looks great! A few changes suggested.

jaeopt · 2022-11-07T17:54:07Z

pkg/odp/config.go

+)
+
+// ConfigState is used to represent state of odp
+type ConfigState int64


Is it common to use int64 for a few states? :)

good catch 👍

jaeopt · 2022-11-07T18:00:14Z

pkg/odp/config.go

+	s.lock.RLock()
+	defer s.lock.RUnlock()
+	value := true
+	if s.odpServiceIntegrated == NotIntegrated {


In server-sdks, we can assume that events are sent only after datafile is parsed. So we do not need to support queueing if "NotDetermined" state (if it can make the implementation simpler).

jaeopt · 2022-11-07T18:06:11Z

pkg/odp/segment_option.go

+package odp
+
+// SegmentOption represents options controlling audience segments.
+type SegmentOption int64


Suggested change

type SegmentOption int64

type OptimizelySegmentOption int64

We'll open this to public. Clients will set options for fetchSegments public api.

jaeopt · 2022-11-07T18:08:45Z

pkg/odp/segment_option.go

+
+const (
+	// IgnoreCache ignores cache (save/lookup)
+	IgnoreCache SegmentOption = iota


Can we use this enum to public? Clients need to specify these options for fetchSegments public api.

yep, anything that starts with a capital letter is public in golang.

jaeopt · 2022-11-07T18:10:26Z

pkg/odp/segment_manager.go

+
+// SegmentManager represents the odp segment manager.
+type SegmentManager interface {
+	FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error)


Suggested change

FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error)

FetchQualifiedSegments(userKey, userValue string, options []OptimizelySegmentOption) (segments []string, err error)

jaeopt · 2022-11-07T18:15:46Z

pkg/odp/segment_manager.go

+}
+
+// NewSegmentManager creates and returns a new instance of DefaultSegmentManager.
+func NewSegmentManager(cacheSize int, cacheTimeoutInSecs int64, odpConfig Config, apiManager SegmentAPIManager) *DefaultSegmentManager {


We also need to support injection of custom SegmentCache (or they can specify size or timeout for default LRUCache). Server-sdks only.

jaeopt · 2022-11-07T18:31:33Z

pkg/odp/segment_manager.go

+
+// FetchQualifiedSegments fetches and returns qualified segments
+func (s *DefaultSegmentManager) FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error) {
+	if s.odpConfig == nil || s.odpConfig.GetAPIHost() == "" || s.odpConfig.GetAPIKey() == "" {


Don't we use odpConfig.odpServiceIntegrated

Makes sense, will update this.

jaeopt · 2022-11-07T18:46:25Z

pkg/odp/config.go

+		apiKey:               apiKey,
+		apiHost:              apiHost,
+		segmentsToCheck:      segmentsToCheck,
+		odpServiceIntegrated: NotDetermined, // initially queueing allowed until the first datafile is parsed


Not sure if we have a use case for that, but - if initial apiKey and apiHost are valid, then odpServiceIntegrated is not consistent.

I have refactored it to not hold states, please have a look.

jaeopt

LGTM with a nit

jaeopt · 2022-11-09T18:21:46Z

pkg/client/optimizely_user_context.go

@@ -38,7 +38,7 @@ type OptimizelyUserContext struct {
 }

 // returns an instance of the optimizely user context.
-func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, qualifiedSegments []string, forcedDecisionService *pkgDecision.ForcedDecisionService) OptimizelyUserContext {
+func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, forcedDecisionService *pkgDecision.ForcedDecisionService, qualifiedSegments []string) OptimizelyUserContext {


We need to differentiate cloning vs new context creation (so we can send IdenfityUser event only for the first creation not for cloing). We can keep it as is and fix it later when we work on the top-level integration.

what change you made for this?

Will be adding this in a separate PR which will be focused on integrating odpManager in userContext.

jaeopt · 2022-11-09T18:30:43Z

pkg/odp/segment_manager.go

+		segmentsCache:     cache,
+	}
+	if segmentManager.segmentsCache == nil {
+		segmentManager.segmentsCache = NewLRUCache(cacheSize, cacheTimeoutInSecs)
 	}


Suggested change

segmentsCache: cache,

}

if segmentManager.segmentsCache == nil {

segmentManager.segmentsCache = NewLRUCache(cacheSize, cacheTimeoutInSecs)

}

segmentsCache: cache ?? NewLRUCache(cacheSize, cacheTimeoutInSecs),

}

golang does not support ternary operators such as ?? or ?:.

msohailhussain

please address my questions before merging. lgtm

msohailhussain · 2022-11-15T02:15:49Z

pkg/client/optimizely_user_context.go

@@ -38,7 +38,7 @@ type OptimizelyUserContext struct {
 }

 // returns an instance of the optimizely user context.
-func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, qualifiedSegments []string, forcedDecisionService *pkgDecision.ForcedDecisionService) OptimizelyUserContext {
+func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, forcedDecisionService *pkgDecision.ForcedDecisionService, qualifiedSegments []string) OptimizelyUserContext {


what change you made for this?

msohailhussain · 2022-11-15T02:18:06Z

pkg/odp/config.go

+}
+
+// DefaultConfig represents default implementation of odp config
+type DefaultConfig struct {


i need to check name.

couldn't name this Config as it was the name of the interface, also ODPConfig is not recommended by linter since the config resides under odp package. Any suggestions?

msohailhussain · 2022-11-15T02:20:53Z

pkg/odp/config.go

+	return s.apiHost
+}
+
+// GetSegmentsToCheck returns value for SegmentsToCheck.


can you explain bit more.

msohailhussain · 2022-11-15T02:22:18Z

pkg/odp/segment_api_manager.go

+
+// SegmentAPIManager represents the segment API manager.
+type SegmentAPIManager interface {
+	FetchSegments(apiKey, apiHost, userKey, userValue string, segmentsToCheck []string) ([]string, error)


just a thought, why we don't pass Config?

good suggestion.

msohailhussain · 2022-11-15T02:23:13Z

pkg/odp/segment_api_manager.go

+}
+
+// FetchSegments returns qualified ODP segments
+func (s *DefaultSegmentAPIManager) FetchSegments(apiKey, apiHost, userKey, userValue string, segmentsToCheck []string) ([]string, error) {


give more proper name instead of s

msohailhussain · 2022-11-15T02:26:01Z

pkg/odp/segment_api_manager.go

+
+// Creates graphql query
+func (s DefaultSegmentAPIManager) createRequestQuery(userKey, userValue string, segmentsToCheck []string) map[string]interface{} {
+	query := fmt.Sprintf(`query($userId: String, $audiences: [String]) {customer(%s: $userId) {audiences(subset: $audiences) {edges {node {name state}}}}}`, userKey)


you don't need fmt.Sprintf.
Please use multiline with indentation to have more visibility.

String interpolation is only supported using fmt.Sprintf, will still look into it.

yasirfolio3 and others added 20 commits October 5, 2022 13:43

LRU cache implemented

11c17ed

Adding cache interface.

85ae342

Merge branch 'master' into yasir/lru-cache

0193ad0

Segment api manager implementated.

60a1a78

fixes.

d76e74c

updating comments.

303e50d

Added support for integrations in datafile and evaluation.

2e2b804

fixes.

e283e8e

Merge branch 'master' into yasir/datafile-parsing-audience-evaluation

b97aab1

fixing race condition.

1c3a3ec

nit fixes.

4b5cb0c

fixes.

960fc07

fixes.

dc881db

1. Audience segments added.

e9ad97b

2. Suggested changes made.

Merge branch 'yasir/datafile-parsing-audience-evaluation' into yasir/…

fed7ebe

…odp-segement-apimanager

Implementations.

e558d03

Unit tests added.

5b94bc8

Tests added.

18faec2

Merge branch 'master' into yasir/odp-segement-apimanager

9e82b09

fixes

d55cf26

yasirfolio3 requested a review from a team as a code owner November 7, 2022 07:56

optimizely-codereview-bot assigned yasirfolio3 Nov 7, 2022

jaeopt requested changes Nov 7, 2022

View reviewed changes

suggested changes made.

cdf8385

yasirfolio3 requested a review from jaeopt November 9, 2022 05:59

jaeopt approved these changes Nov 9, 2022

View reviewed changes

msohailhussain approved these changes Nov 15, 2022

View reviewed changes

yasirfolio3 added 3 commits November 15, 2022 12:10

suggested fixes made.

be5e080

fixes.

7dc9c44

fixes.

201eacc

msohailhussain merged commit c99e8dd into master Nov 16, 2022

msohailhussain deleted the yasir/odp-segement-apimanager branch November 16, 2022 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(segment-manager): Adds implementation for ODPSegmentManager #353

feat(segment-manager): Adds implementation for ODPSegmentManager #353

yasirfolio3 commented Nov 7, 2022

jaeopt left a comment

jaeopt Nov 7, 2022

yasirfolio3 Nov 8, 2022

jaeopt Nov 7, 2022

jaeopt Nov 7, 2022

jaeopt Nov 7, 2022

yasirfolio3 Nov 8, 2022 •

edited

Loading

jaeopt Nov 7, 2022

jaeopt Nov 7, 2022

jaeopt Nov 7, 2022

yasirfolio3 Nov 8, 2022 •

edited

Loading

jaeopt Nov 7, 2022

yasirfolio3 Nov 8, 2022

jaeopt left a comment

jaeopt Nov 9, 2022

msohailhussain Nov 15, 2022

yasirfolio3 Nov 15, 2022

jaeopt Nov 9, 2022

yasirfolio3 Nov 9, 2022

msohailhussain left a comment

msohailhussain Nov 15, 2022

msohailhussain Nov 15, 2022

yasirfolio3 Nov 15, 2022

msohailhussain Nov 15, 2022

msohailhussain Nov 15, 2022

yasirfolio3 Nov 15, 2022

msohailhussain Nov 15, 2022

msohailhussain Nov 15, 2022

yasirfolio3 Nov 15, 2022

	FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error)
	FetchQualifiedSegments(userKey, userValue string, options []OptimizelySegmentOption) (segments []string, err error)

feat(segment-manager): Adds implementation for ODPSegmentManager #353

feat(segment-manager): Adds implementation for ODPSegmentManager #353

Conversation

yasirfolio3 commented Nov 7, 2022

Summary

Test plan

Issues

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 Nov 8, 2022 •

edited

Loading

yasirfolio3 Nov 8, 2022 •

edited

Loading