-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
1 similar comment
return | ||
end | ||
begin | ||
@event_queue.push(user_event, true) |
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.
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
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.
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.
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.
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) |
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.
@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.
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, would like @msohailhussain to also take a look
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. please address comments.
return | ||
end | ||
begin | ||
@event_queue.push(user_event, true) |
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.
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 |
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.
Can you pleae add logs in flush_queue
while flushing.
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.
Done
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.
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 |
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 begin/end wrapper is unnecessary.
@event_queue.push(user_event, true) | ||
rescue Exception | ||
@logger.log(Logger::WARN, 'Payload not accepted by the queue.') | ||
return |
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.
This return redundant.
end | ||
begin | ||
@event_queue.push(user_event, true) | ||
rescue Exception |
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.
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.)
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
Issues