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

time support #57

Merged
merged 14 commits into from
Sep 19, 2019
Merged

time support #57

merged 14 commits into from
Sep 19, 2019

Conversation

BillDorn
Copy link
Contributor

@BillDorn BillDorn commented Aug 16, 2019

@redonkulus
Adds support for time based configuration.
Scheduled configurations may be defined in the following way

{
  settings: {
    dimensions: ["environment:staging,test", "device:smartphone"],
    schedule: {
      start: "1993-10-30T13:33:38.612Z",
      end: "1996-08-03T17:00:14.281Z"
    }
  },
  host: "stage.example.com",
  prefix: "m."
}

Either start or end may be omitted to define a one sided interval. Any string that can be parsed by the builtin Date and converted to an epoch timestamp should work.

To make use of scheduled configs you must use readTimeAware or readNoMergeTimeAware, this take a context and a timestamp in milliseconds.

Time configs are merged in order of start time.

Caching

To support caching you may pass the option cachInfo: true on time aware calls. Ycb will add a field at YCB.EXPIRATION_KEY on the returned config denoting the next time this value will change. If it will not change in the future then this field is not added.
For non-merging reads this field will be added to the first config only.
The new method getCacheKey(context) converts a context object into a string. The output may only be equal if corresponding read calls would return the same result.


I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Collaborator

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

Overall the code changes look good. I left some general code comments, nothing major.

Can you also add a section in the readme detailing the scheduling feature and code samples of how to use it?

}
var value = context[depth];
if(!Array.isArray(value)) {
var keys = this.precedenceMap[value];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you consolidate this code with code from lines 213-228? Looks like they do the same thing.

@@ -236,7 +439,7 @@ Ycb.prototype = {
_contextToObject: function(context) {
var contextObj = {};
for(var i=0; i<context.length; i++) {
if(context[i] !== '0') {
if(context[i] !== 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it a number now?

Copy link
Contributor Author

@BillDorn BillDorn Aug 30, 2019

Choose a reason for hiding this comment

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

The context array is built from the map keys which are numbers, before these were js objects so the keys were getting converted to strings.

@@ -52,6 +54,14 @@ function omit(obj, omitKey) {
}, {});
}

function logBundleWarning(index, message, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of just warning, do we want to throw at all? i guess in the context of a live config scenario, you wouldn't want to just start breaking the server if a config is formatted incorrectly.

Copy link
Contributor Author

@BillDorn BillDorn Aug 30, 2019

Choose a reason for hiding this comment

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

This is patterned after the older version's behavior. Seems safer to match that at least by default. If there is a use case where calling code would want to handle these errors somehow we could add an option to throw. I assume live config could catch this and simply not swap in the broken config.

index.js Outdated Show resolved Hide resolved
continue;
}
if(!configObj.settings) {
if(!configObj.dimensions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

dimensions is not required right?, the old syntax should still work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check prevents an erroneous warning log from the dimensions object in the bundle.

index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
cmp([0,7,0], ycb._parseContext({region:'fr'}));
cmp([0,6,9], ycb._parseContext({flavor:'bt', region:'ir'}));
cmp([2,7,8], ycb._parseContext({lang:'fr_FR', region:'fr', flavor:'att'}));
cmp([0,4,0], ycb._parseContext({region:'fr'}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did these change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused dimension values with used ancestor values were getting assigned distinct labels to even though they are functionally equivalent to their ancestor. This change maps them to the ancestor instead.

index.js Show resolved Hide resolved
Copy link
Collaborator

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

Thanks for adding this in, great job!

@redonkulus redonkulus merged commit 6d5940e into yahoo:master Sep 19, 2019
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.

2 participants