-
Notifications
You must be signed in to change notification settings - Fork 32
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(decide): add a new set of decide apis #406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The required change is to the documentation. The others are strong recommendations to flatten the package and to use a List
as opposed to an array.
core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Lgtm |
copiedAttributes, | ||
projectConfig, | ||
allOptions, | ||
decisionReasons); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that decisionReasons
is expected to be modified from inside. In my experience, this kind of flow gets a little confusing when someone is following the code flow. On suggestion can be to return Pair<Decision, Reasons>
fromgetVariationForFeature
. This will clearly tell that decision reasons are also coming back from this function and original object will also be intact causing no confusion. what do you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good point! We also consider that option as well. It's dropped since it's a bit complicated to process and merge (and/or audience, etc) at each stage. We can find better solutions. I think you can try with other sdks with your suggestion and I can come back to this if we see a good solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments below. It seems like a bad pattern to be passing around a parameter that is constantly being augmented. We should not propagate this pattern to all SDKs. And a nit to remove the star for including decision package.
@@ -34,6 +34,7 @@ | |||
import com.optimizely.ab.optimizelyconfig.OptimizelyConfig; | |||
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager; | |||
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService; | |||
import com.optimizely.ab.optimizelydecision.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it is bad practice to use star in imports. As you can see from optimizelyconfig above it, importing the specific classes is much more desirable and easier to follow. Thanks!
public Boolean evaluate(ProjectConfig config, | ||
Map<String, ?> attributes, | ||
List<OptimizelyDecideOption> options, | ||
DecisionReasons reasons) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaeopt I want to second this. It also follows the point above about passing in parameters that are getting augmented. https://github.com/optimizely/java-sdk/pull/406/files#r520122062
This is a bad pattern that we are going to propagate to all our sdks.
https://github.com/optimizely/intellij-plugin/blob/master/src/main/resources/logback.xml
The above is an example of using an appender in slf4j (using logback but you don't need it https://stackoverflow.com/questions/28043566/capture-log4j-output). It is one example of how you could use aop to grab the log output when reasons is "on." I'm also wondering if there is any potential issues with threading since your reasons are not thread safe.
@thomaszurkan-optimizely I'm wondering why you think it's not thread-safe. Each decide-api call in a thread will keep track of its own reason instance, so should be thread-safe. |
Although I disagree with the implementation, I won't block it.
Summary
Add a new set of Decide APIs: