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

Changed LPOP to BLPOP in file redis_store.go #63

Closed
wants to merge 2 commits into from

Conversation

samuelebistoletti
Copy link
Contributor

I created a new pull request with BLPOP change in redis_store.go as requested yesterday. Timeout interval is now configurable in config.yml.

@@ -37,6 +37,9 @@ redis_activity_consumers: 8
# Number of concurrent connections to Redis in the connection pool.
redis_connection_pool_size: 20

# Max number of seconds BLPOP waits before returning anyway. Must be a positive integer. 0 means wait indefinitely.
redis_blpop_timeout: 10

Choose a reason for hiding this comment

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

why not wait indefinitely by default so when a message is there incus resolves it instantly? (unless this blocks redis somehow ofc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I wanted a "safe" value for all the possible environments, I'm not sure that zero has no side effects. Anyway the change is trivial so let me know if I have to change it to zero.

Choose a reason for hiding this comment

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

not a redis user so no idea if it would be safe or not, why I asked :P

Choose a reason for hiding this comment

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

I think this should be 0, not sure about the polling implementation but either you repoll on timeout or just blpop 0, also make sure to use a different connection for this since the command will block the whole connection so you can't run any other commands on it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I closed this pr, new pr opened with this value with default at 0.

Look here #73

Choose a reason for hiding this comment

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

you now you can just add commits to the branch and the PR will update right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake, sorry

Choose a reason for hiding this comment

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

no problem

@AdriVanHoudt
Copy link

:shipit:

@samuelebistoletti samuelebistoletti changed the title Changed LPOP to BLPOP in redis_store.go Changed LPOP to BLPOP in file redis_store.go Oct 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants