Skip to content
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

Update Valkey keyword in sentinel.conf #171

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

hwware
Copy link
Member

@hwware hwware commented Apr 3, 2024

Mostly comments, but one pre-filled config in this template config file is changed:

pidfile /var/run/valkey-sentinel.pid

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Looks good, but DCO is missing and there is some merge conflict.

sentinel.conf Outdated
@@ -145,8 +142,8 @@ sentinel down-after-milliseconds mymaster 30000
# user worker +@admin +@connection ~* on >ffa9203c493aa99
#
# For more information about ACL configuration please refer to the Redis
# website at https://redis.io/topics/acl and redis server configuration
# template redis.conf.
# website at https://redis.io/topics/acl and valkey server configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as our website will have the same URLs, I think this link will work. This pages are defined as files in valkey-doc.

Suggested change
# website at https://redis.io/topics/acl and valkey server configuration
# website at https://valkey.io/topics/acl and valkey server configuration

Copy link
Member Author

Choose a reason for hiding this comment

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

Address your comments, and I update redis.io to valkey.io in 2 places, pls take a look. Thanks

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

Some questions though, but nothing that we need to fix.

Comment on lines -17 to +20
# When running daemonized, Redis Sentinel writes a pid file in
# /var/run/redis-sentinel.pid by default. You can specify a custom pid file
# When running daemonized, Valkey Sentinel writes a pid file in
# /var/run/valkey-sentinel.pid by default. You can specify a custom pid file
# location here.
pidfile /var/run/redis-sentinel.pid
pidfile /var/run/valkey-sentinel.pid
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this is the default pidfile only if this config file is used. If sentinel is started without this config, the default is this one in server.c:

#define CONFIG_DEFAULT_PID_FILE "/var/run/redis.pid"

If this is true, then at least it's not a new issue, so we don't have to change it today. It would just be interesting to know if this is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are right. It works like this:
if (background || server.pidfile) createPidFile(); (server.c)

sentinel.conf Outdated Show resolved Hide resolved
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
@zuiderkwast
Copy link
Contributor

@valkey-io/core-team Include this in the 7.2.x release?

@zuiderkwast zuiderkwast merged commit 29621bc into valkey-io:unstable Apr 4, 2024
14 checks passed
@hwware
Copy link
Member Author

hwware commented Apr 4, 2024

pidfile

any reason not include in 7.2 release?

@zuiderkwast
Copy link
Contributor

@hwware We did a similar pidfile change in #29, so I think this is OK.

madolson pushed a commit to madolson/valkey that referenced this pull request Apr 7, 2024
Mostly comments, but one pre-filled config in this template config file
is changed:

    pidfile /var/run/valkey-sentinel.pid

---------

Signed-off-by: hwware <wen.hui.ware@gmail.com>
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Mostly comments, but one pre-filled config in this template config file
is changed:

    pidfile /var/run/valkey-sentinel.pid

---------

Signed-off-by: hwware <wen.hui.ware@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants