-
Notifications
You must be signed in to change notification settings - Fork 9
NOISSUE - Make Check Interval Configurable #277
Conversation
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.
Did you try to spawn fewer routines (say, 100, instead of 10k), and wait shorter (say, 3s instead of 1m)? Also, consider replacing simple wait with the actual consumer (while this will definitely make this test even more "integration") so that we wait for all the events to be consumed properly.
TestUnavailablePublish
due to Uncertainties110dd1b
to
cc5bc99
Compare
pkg/events/redis/publisher.go
Outdated
} | ||
|
||
func NewPublisher(ctx context.Context, url, stream string) (events.Publisher, error) { | ||
func NewPublisher(ctx context.Context, url, stream string, unpublishedEventsCheckInterval time.Duration) (events.Publisher, error) { |
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.
Rename to retryPeriod
or just retry
- time.Duration
type implies the purpose.
defer ticker.Stop() | ||
|
||
for { | ||
select { | ||
case <-ticker.C: | ||
if err := es.checkRedisConnection(ctx); err == nil { | ||
if err := es.checkConnection(ctx); err == nil { |
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.
Any reason for reversing the order here?
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 order hasn't changed only renaming
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.
I meant, on the line below; we use reverse order to publish events. Is there a reason we use reverse order?
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.
First event published will be the first event to be republished
Removes TestUnavailablePublish due to uncertainity when running tests on CI. It also removes the code to generate random events, spawn goroutines for publishing, and test unavailable publish when the message broker is unreachable. Signed-off-by: Rodney Osodo <[email protected]>
This commit adds a new test case to the publisher. It also removes a method that is no longer needed. The code in this file handles publishing and subscribing to events using RabbitMQ and Redis. It includes logic for pausing and unpausing a container, generating random events, and checking the number of received events. Furthermore, an argument is added to the NewPublisher function to allow specifying an interval for checking unpublished events. Signed-off-by: Rodney Osodo <[email protected]>
Signed-off-by: rodneyosodo <[email protected]>
What type of PR is this?
This is a bug fix because it fixes failing tests for events package
What does this do?
This PR makes check interval for unpublished events configurable for redis since Redis is the only event source than that doesn't support unavailable publish i.e store events to a buffer if the server is down
Which issue(s) does this PR fix/relate to?
No issue
Have you included tests for your changes?
Yes, I have included tests for my changes.
Did you document any new/modified feature?
No
Notes