8000 refact: Batch Event Processor and Unit Tests by oakbani · Pull Request #215 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]

Skip to content

refact: Batch Event Processor and Unit Tests #215

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 8 commits into from
Oct 29, 2019

Conversation

oakbani
Copy link
Contributor
@oakbani oakbani commented Oct 17, 2019

Summary

Addresses
1. Event Batching thread can sleep anytime from 50 ms to 100ms. In other SDKs we strictly sleep for 50ms. This makes it inconsistent.
2. Mutex applied over Queue which already implements locking mechanism internally and is made for multithreaded access.
3. Unit tests filled up with varying sleep timers.
4. Unit tests throwing errors due to doubles/spys being passed to internal threads.
5. A message being logged in debug mode in a while loop. This fills up all logs and a client will certainly report it
_

Test plan

  • Previous unit tests modified and new added.

Issues

  • OASIS-5495

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.086% when pulling b25ec6f on oakbani/refact-event-processor into 99512e4 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.086% when pulling b25ec6f on oakbani/refact-event-processor into 99512e4 on master.

@coveralls
Copy link
coveralls commented Oct 17, 2019

Coverage Status

Coverage increased (+0.02%) to 99.516% when pulling 6f44678 on oakbani/refact-event-processor into 5d3c74d on master.

@oakbani oakbani changed the title [WIP] refact: Remove mutex from batch event processor refact: Batch Event Processor and Unit Tests Oct 21, 2019
@oakbani oakbani marked this pull request as ready for review October 21, 2019 14:52
@oakbani oakbani requested a review from a team as a code owner October 21, 2019 14:52
return
end
begin
@event_queue.push(user_event, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting 2nd arg to true so that the main thread doesn't block and an exception is raised if the Queue is full.
https://ruby-doc.org/core-2.3.0_preview1/SizedQueue.html#method-i-push

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 make sure in other SDKs, we are also getting queue size, but here we are just setting default queue size. Can you please add attribute to support that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are already allowing user to pass it's own queue. Like we do in spec file, send our own SizedQueue with different size.


@logger.log(Logger::INFO, 'Stopping scheduler.')
@event_queue << SHUTDOWN_SIGNAL
@thread.join(DEFAULT_TIMEOUT_INTERVAL)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thread.exit would kill the inner thread and we may miss out events in the buffer. .join would instead wait for the thread to complete up to the given timeout.

Copy link
Contributor
@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm, would like @msohailhussain to also take a look

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. please address comments.

return
end
begin
@event_queue.push(user_event, true)
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 make sure in other SDKs, we are also getting queue size, but here we are just setting default queue size. Can you please add attribute to support that.

)
flush_queue!
end
flush_queue! if Helpers::DateTimeUtils.create_timestamp > @flushing_interval_deadline
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 pleae add logs in flush_queue while flushing.

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

@mikeproeng37 mikeproeng37 merged commit 6e84924 into master Oct 29, 2019
@mikeproeng37 mikeproeng37 deleted the oakbani/refact-event-processor branch October 29, 2019 18:38
Copy link
Contributor
@jasonkarns jasonkarns left a comment

Choose a reason for hiding this comment

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

Sorry for the rando review. I just stumbled on this as I've been reading the source while integrating optimizely.

Two unimportant nits, but the rescue Exception issue should be fixed before next release.

@logger.log(Logger::WARN, 'Payload not accepted by the queue.')
return
end
begin
Copy link
Contributor

Choose a reason for hiding this comment

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

The begin/end wrapper is unnecessary.

@event_queue.push(user_event, true)
rescue Exception
@logger.log(Logger::WARN, 'Payload not accepted by the queue.')
return
Copy link
Contributor

Choose a reason for hiding this comment

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

This return redundant.

end
begin
@event_queue.push(user_event, true)
rescue Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be rescuing Exception. That will prevent the ruby process from safely exiting on exit and other base Exception subclasses. (should only be rescuing StandardError, which is the default behavior when no exception class is provided.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0