refact: Batch Event Processor and Unit Tests #215
Conversation
1 similar comment
| return | ||
| end | ||
| begin | ||
| @event_queue.push(user_event, true) |
There was a problem hiding this comment.
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.
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.
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.
@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.
| return | ||
| end | ||
| begin | ||
| @event_queue.push(user_event, true) |
There was a problem hiding this comment.
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.
Can you pleae add logs in flush_queue while flushing.
| @logger.log(Logger::WARN, 'Payload not accepted by the queue.') | ||
| return | ||
| end | ||
| begin |
There was a problem hiding this comment.
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 |
| end | ||
| begin | ||
| @event_queue.push(user_event, true) | ||
| rescue Exception |
There was a problem hiding this comment.
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