Skip to content

feat(eventProcessor): Incorporate batch processing into OptimizelyFactory. #198

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

Merged
merged 9 commits into from
Sep 18, 2019

Conversation

rashidsp
Copy link
Contributor

@rashidsp rashidsp commented Aug 30, 2019

Summary

  • Incorporate BatchEventProcessor with OptimizelyFactory.

Test plan

  • Added unit tests

@rashidsp rashidsp requested a review from a team as a code owner August 30, 2019 15:26
@rashidsp rashidsp added the WIP Work in progress label Aug 30, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 99.431% when pulling eca354d on rashid/EP-integrate-Optly-factory into 0aa4a7e on rashid/Batch-EP-add-logEvent.

@coveralls
Copy link

coveralls commented Aug 30, 2019

Coverage Status

Coverage increased (+0.006%) to 99.431% when pulling e5b1547 on rashid/EP-integrate-Optly-factory into 31e93c7 on master.

@rashidsp rashidsp removed the WIP Work in progress label Aug 30, 2019
@mjc1283
Copy link

mjc1283 commented Sep 5, 2019

@rashidsp Could this be opened against master after merging #197 and/or #200?

@rashidsp
Copy link
Contributor Author

rashidsp commented Sep 5, 2019

@rashidsp Could this be opened against master after merging #197 and/or #200?

@mjc1283 against master, i will change the base and update it with master.

Copy link

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

Requesting changes to get it out of queues until it's based on master.

@rashidsp rashidsp changed the base branch from rashid/Batch-EP-add-logEvent to master September 6, 2019 17:45
@rashidsp rashidsp requested a review from mjc1283 September 6, 2019 17:46
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.

few minor comments.

unless batch_size.is_a? Integer
logger.log(
Logger::ERROR,
'Batch size has invalid type. Reverting to default configuration.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this message to
Batch size is invalid, setting to default batch size {batchSize}

unless batch_size.positive?
logger.log(
Logger::ERROR,
'Batch size cannot be <= 0. Reverting to default configuration.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Batch size is negative, setting to default batch size {batchSize}

unless flush_interval.is_a? Numeric
logger.log(
Logger::ERROR,
'Flush interval has invalid type. Reverting to default configuration.'
Copy link
Contributor

Choose a reason for hiding this comment

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

please change these msgs too.

unless flush_interval.positive?
logger.log(
Logger::ERROR,
'Flush interval cannot be <= 0. Reverting to default configuration.'
Copy link
Contributor

Choose a reason for hiding this comment

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

this one as well. @oakbani

@@ -57,6 +105,13 @@ def self.custom_instance(
config_manager = nil,
notification_center = nil
)
event_processor = BatchEventProcessor.new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add in a doc, default batchsize and flush_interval will be used to setup batchEventProcessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mjc1283
Copy link

mjc1283 commented Sep 17, 2019

@msohailhussain Could you please review the updates from @rashidsp ?

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

@mjc1283 mjc1283 merged commit 7ab4650 into master Sep 18, 2019
@rashidsp rashidsp deleted the rashid/EP-integrate-Optly-factory branch September 18, 2019 17:53
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.

4 participants