Skip to content
This repository has been archived by the owner on Oct 14, 2024. It is now read-only.

NOISSUE - Make Check Interval Configurable #277

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

rodneyosodo
Copy link
Member

@rodneyosodo rodneyosodo commented Jan 23, 2024

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

Copy link
Contributor

@dborovcanin dborovcanin left a 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.

@rodneyosodo rodneyosodo changed the title NOISSUE - Remove TestUnavailablePublish due to Uncertainties NOISSUE - Make Check Interval Configurable Jan 24, 2024
@rodneyosodo rodneyosodo force-pushed the events branch 3 times, most recently from 110dd1b to cc5bc99 Compare January 25, 2024 08:10
}

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) {
Copy link
Contributor

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.

pkg/events/redis/publisher.go Outdated Show resolved Hide resolved
defer ticker.Stop()

for {
select {
case <-ticker.C:
if err := es.checkRedisConnection(ctx); err == nil {
if err := es.checkConnection(ctx); err == nil {
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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

rodneyosodo and others added 3 commits January 25, 2024 15:41
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]>
@dborovcanin dborovcanin merged commit 75b65af into absmach:main Jan 25, 2024
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants