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: Added set polling interval and set blocking timeout period method in OptimizelyFactory #264

Merged
merged 4 commits into from
May 12, 2021

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented May 6, 2021

Summary

  • Polling interval and blocking timeout period of configManager can be now set using optimizelyFactory and the provided intervals will be given priority over config file values.

Test plan

All Unit tests and FSC tests should pass.

Issues

var configManager = builder
.WithSdkKey(httpProjectConfigElement.SDKKey)
.WithUrl(httpProjectConfigElement.Url)
.WithFormat(httpProjectConfigElement.Format)
.WithPollingInterval(TimeSpan.FromMilliseconds(httpProjectConfigElement.PollingInterval))
.WithBlockingTimeoutPeriod(TimeSpan.FromMilliseconds(httpProjectConfigElement.BlockingTimeOutPeriod))
.WithPollingInterval(PollingInterval == TimeSpan.Zero ? TimeSpan.FromMilliseconds(httpProjectConfigElement.PollingInterval) : PollingInterval)
Copy link
Contributor

Choose a reason for hiding this comment

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

PLease update header.

};

Assert.AreEqual(actualConfigManagerProps, expectedConfigManagerProps);

optimizely.Dispose();
}

[Test]
Copy link
Contributor

Choose a reason for hiding this comment

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

add one more priority test. when polling interval is set using method then configelement value is discarded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these tests already show that configElement values will be discarded once we set the polling interval using optimizelyFactory.
Value given in ConfigElement is 2 second, where as we can see that at line 130 and 116 after setting polling Interval using OptlyFactory all other values remained same only polling and blocking interval are changed.

@@ -78,8 +77,8 @@ public void TestProjectConfigManagerUsingSDKKey()
Url = "https://cdn.optimizely.com/datafiles/my-sdk-key.json",
LastModified = "",
AutoUpdate = true,
BlockingTimeout = TimeSpan.FromSeconds(15),
PollingInterval = TimeSpan.FromMinutes(5)
BlockingTimeout = TimeSpan.FromSeconds(30),
Copy link
Contributor

Choose a reason for hiding this comment

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

update header.

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

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.

It looks good. One concern is we have 3 similar factory methods, which can have totally different config data paths (see comments) for timeout values. Can we merge and make them consistent?

Comment on lines +142 to +143
.WithPollingInterval(PollingInterval)
.WithBlockingTimeoutPeriod(BlockingTimeOutPeriod)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused - why don't have the config reading unlike the above:
.WithPollingInterval(PollingInterval == TimeSpan.Zero ? TimeSpan.FromMilliseconds(httpProjectConfigElement.PollingInterval) : PollingInterval)
This can have different configuration paths depending on factory method parameters. It can be confusing unless we document very clearly about them.

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

@jaeopt jaeopt merged commit 25ad73c into master May 12, 2021
@jaeopt jaeopt deleted the mnoman/OptimizelyFactoryAdditional branch May 12, 2021 22:31
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