Remove trademarked wording on configuration file and individual configs#29
Conversation
madolson
left a comment
There was a problem hiding this comment.
@zuiderkwast You made the .json files mostly agnostic to Redis, do you think we should do the same here?
| createStringConfig("cluster-announce-hostname", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_hostname, NULL, isValidAnnouncedHostname, updateClusterHostname), | ||
| createStringConfig("cluster-announce-human-nodename", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_human_nodename, NULL, isValidAnnouncedNodename, updateClusterHumanNodename), | ||
| createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL), | ||
| createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "placeholderkv", NULL, NULL), |
There was a problem hiding this comment.
Is this fine or should we replace it with server ?
There was a problem hiding this comment.
Can you look into what syslog-ident does? This could also be a breaking change.
There was a problem hiding this comment.
The ident string is the first parameter passed to openlog(3) - its a string that will be prepended to every logged message. From the man page:
The string pointed to by ident is prepended to every message, and is typically set to the program name. If ident is NULL, the program name is used. (POSIX.1-2008 does not specify the behavior when ident is NULL.)
Probably the new name makes most sense here IMO, although it is arguably a subtle breakage for any 3rd party software searching for the string "redis" in system logs. Depends on what level of compatibility is sought I guess.
There was a problem hiding this comment.
@hpatro I'm okay changing this, but let's create an issue tracking all of the little decisions we are looking at making.
There was a problem hiding this comment.
Added to the top comment.
There was a problem hiding this comment.
Can we use a macro here, like SERVER_NAME instead of "placeholderkv" and define it in one place in server.h so it's easy to change later?
There was a problem hiding this comment.
echoing on the "breakage" concern. It can break the tooling like log parsers.
Can we use a macro here, like SERVER_NAME instead of "placeholderkv" and define it in one place in server.h so it's easy to change later?
this is a great idea. we can also introduce either conditional compilation or even new config so that at least there is a grace period for the existing users to upgrade their tools (log parsers, for instance). The "compat" shim can be removed in future releases.
|
@madolson Tried making the comments generic. PTAL. |
madolson
left a comment
There was a problem hiding this comment.
Mostly just the main config file is broken english.
| createStringConfig("cluster-announce-hostname", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_hostname, NULL, isValidAnnouncedHostname, updateClusterHostname), | ||
| createStringConfig("cluster-announce-human-nodename", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_human_nodename, NULL, isValidAnnouncedNodename, updateClusterHumanNodename), | ||
| createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL), | ||
| createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "placeholderkv", NULL, NULL), |
There was a problem hiding this comment.
Can you look into what syslog-ident does? This could also be a breaking change.
| * The following additional flags are only used in order to put commands | ||
| * in a specific ACL category. Commands can have multiple ACL categories. | ||
| * See redis.conf for the exact meaning of each. | ||
| * See placeholderkv.conf for the exact meaning of each. |
There was a problem hiding this comment.
Would be nice to refactor this information about categories out of the config file, which seems like an odd place, but probably not in scope here.
|
@placeholderkv/core-team Shall we maintain an issue with list of new changes or should we just attach a label |
zuiderkwast
left a comment
There was a problem hiding this comment.
I agree it's good to be name agnostic in many places, such as in all comments, replace Redis with "the server" and similar.
The config file can very well be called server.conf.
But some global names like pidfile and socket file would need to be more unique (as commented) and what about the binaries? It'd be silly to just call them server, cli, benchmark, right? I think we need a name here.
Let's think about what we want here. The name "redis" has polluted the code base, which turned into a problem now. Do we want to do the same with another name? Is our name going to be a trademark that can cause trouble for others?
If we want to make it as easy as possible to change the name, we could define it in only one place, for example in the Makefile, then passed to the compiler as -DNAME=placeholderkv.
I think we should keep pid and socket files to be specific to the engine. I agree with moving config -> server.conf. The server/cli/benchmark should have some prefix, but that will depend on the name we end up on. I agree with -DName approach for now. |
| src/configs | ||
| redis.ds | ||
| src/redis.conf | ||
| src/placeholderkv.conf |
There was a problem hiding this comment.
I had a discussion with @zuiderkwast. we should create symlinks for these files for two reasons
- to reduce the size of the pr and we can change the names in the rest of the codebase gradually
- to maintain compat. I think the "redis" string is spread out in the whole ecosystem already.
There was a problem hiding this comment.
IIUC, you're suggesting to have a follow up task to create a symlink to redis-* binaries with valkey-* binaries and the config file. Is that right?
There was a problem hiding this comment.
For the configuration file there is no makefile-based install currently (so nowhere really to create this new symlink) - on Linux this is handled by the distributions in their rpm spec / debian rules files. On rpm systems (at least, maybe deb also) its slightly more complex in that there's also a directory created for the config files - i.e. redis.conf and sentinel.conf are placed in /etc/redis (so the directory will need to be symlinked too).
For valkey I recommend leaving this part of the symlinking to the distributors, but I agree it'll be good to install compatibility symlinks for the commands.
There was a problem hiding this comment.
Thanks for the insight into Linux @natoscott
I also did think about putting the symlink creation as part of Makefile build.
But then I thought there could also be users who directly download the release from github and start working on it and redis.conf would be missing until they perform a build. So, proceeded with this approach.
There was a problem hiding this comment.
@natoscott, I like your idea of leaving the symlinks to the distributors but curious to know why you don't think creating symlinks in the makefile would work?
I think there are 3 ways for a user to get access to the valkey binaries
- pre-installed
- pre-built docker container
- github repo and DIY
I would assume that all end users fall in the first 2 categories while the 3rd bucket is probably all developers. the compat bars will be different for these two personas, end users vs developers, with the bar a lot higher for the end user. So maybe having the distributors (including docker container distributors) own up the symlinks is a better idea? developers should be able to figure out where the files are quickly.
|
@zuiderkwast @PingXie Thanks for the review. Could you PTAL once again? I've added a compile time param |
Thanks for reducing the scope of this pr. I think we still need a uber decision/guidance on file name changes:
|
I'm inclined with 2. |
I am fine with option 2 as well. Can you propose the changes in this pr? Normally I would be in favor of smaller PRs but since we will need to cherry-pick these changes into a 7.2 branch, I think it is important that every pr is self-contained so we don't have to deal with dependency and order. |
I don't see a follow up discussion on this questions. Tagging is fine. No need to denormalize the information. I prefer light processes. |
|
@PingXie I've created a symlink (source - |
I guess we aren't breaking anything with this PR anymore. Agreed let's keep the process light. Release owner would need to visit the PR(s) marked with |
zuiderkwast
left a comment
There was a problem hiding this comment.
OK, this is starting to look good. It's mainly about the config file.
Let's do other changes in other PRs.
|
@zuiderkwast @PingXie @hwware PTAL. @hwware Will address the other feedback in a separate PR. |
|
Moving the config related changes to |
zuiderkwast
left a comment
There was a problem hiding this comment.
Mostly LGTM.
There are a few occurrences of "server" which should be "valkey", commented by @PingXie.
zuiderkwast
left a comment
There was a problem hiding this comment.
I applied some changes I think you missed in the jungle of comments.
Let's merge this?
Once DCO pass, pls meger it. LGTM |
|
@zuiderkwast @PingXie @hwware Could we do a squash and merge through github and add sign off in that commit? |
ofc I can do that, asking if we could avoid it and directly go for the squash/merge in github. |
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
|
There are some merge conflicts with unstable. performing the squash locally and force pushing here. |
|
@valkey-io/core-team Sadly the |
|
@hpatro Is the top comment updated? No other changes than config file and pidfile? (It's not the default but only a suggested pidfile in the conf file.) |
|
co-authored-by who exactly? I think it doesn't matter in this case. We can skip it. |
The only meaningful change is rename of
It was you :) |
|
If people use redis.conf (valkey.conf) in scripts and maybe change something using sed or so, they would be affected by the pid file. In the commit message when merging I mentioned it: |
…gs (valkey-io#29) Remove trademarked wording on configuration layer. Following changes for release notes: 1. Rename redis.conf to valkey.conf 2. Pre-filled config in the template config file: Changing pidfile to `/var/run/valkey_6379.pid` Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
…gs (valkey-io#29) Remove trademarked wording on configuration layer. Following changes for release notes: 1. Rename redis.conf to valkey.conf 2. Pre-filled config in the template config file: Changing pidfile to `/var/run/valkey_6379.pid` Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
valkey-io#29) * [S2.9 / C1] Config plumbing: master-switch + active-sweeper + interval Implements the declarative master-switch model from PR-B (design doc): replaces the bool `compression-enabled` config with the 3-state enum `compression-master-switch` (compression / decompression / off, default off), adds the `compression-active-sweeper` enum (enabled / disabled, default disabled) and `compression-active-sweeper-interval` time config (default 0 = no periodic re-runs). Matching INFO fields. Apply hooks implementing the R2.1.5 transition rules and the R2.1.6 non-functional state warning. C1 stops at config plumbing; the transient-view drain mode dispatch is C2, the sweeper engine + COMPRESSION SWEEP FORCE command are C3. Master-switch transition table (R2.1.5): compression -> decompression : auto-retire active dict (drain mode) compression -> off : NO retire (state frozen) off -> decompression : auto-retire active dict if any decompression -> off : no action (already retired) off -> compression : no action; existing dict reusable decompression -> compression : no action; falls into R2.1.7 third state Apply-hook implementation notes: - The hooks track the prior value via file-static caches (one for master, one for active-sweeper, one shared for the warning detector + a parallel for compression-threads). compressionInit() calls syncApplyHookCaches() AFTER boot config is applied, which fixes two bugs: 1. Boot-time non-functional warning (master=compression+threads=0 set at boot) didn't fire because apply hooks don't run during boot config load. 2. An operator booting with master=compression and later setting master=decompression would have seen the apply hook log a misleading "off -> decompression" transition (because the static was initialized to the default, not the boot value). - applyCompressionThreads in config.c calls compressionAfterThreadsApplied to keep the warning detector's threads cache in sync. - maybeWarnNonFunctional emits LL_WARNING once on transition INTO master=compression+threads=0; subsequent CONFIG SETs that don't leave the state are no-ops. Stale-reference sweep (PR-B vocabulary update): - src/compression.c: 4 comments and 2 production checks updated from "compression-enabled" / server.compression_enabled to master-switch language. INFO field rendering switched from compression_enabled:0 to compression_master_switch:<name> + new compression_active_sweeper + compression_active_sweeper_interval fields. - src/compression_workers.c: 1 comment line. - src/compression_train.c: gated cron entry on master==compression instead of !enabled bool. - src/compression_registry.h: 2 comments referencing "COMPRESSION SWEEP direction=decompress" updated to the new operator surface (master=decompression + active-sweeper=enabled). - src/compression_registry.c: log message at dict-cap-reached. - src/compression.h: removed unused compressionToggle() declaration (was a Phase-0 stub, never called); replaced with the apply-hook declarations. Test updates: - src/unit/test_compression_eligibility.cpp: replaced server.compression_enabled with server.compression_master_switch using the new enum constants. Added a new test case MasterSwitchDecompressionBlocksEligibility verifying that the predicate also rejects in master=decompression (drain mode). - src/unit/test_compression_train.cpp: same conversion. - src/unit/test_compression_workers.cpp: 1 comment update. - tests/unit/type/compression.tcl: rewrote 5 tests that referenced compression-enabled to use compression-master-switch. Added two new tests verifying the enum accepts each documented value plus rejects out-of-set values for both master-switch and active-sweeper. Updated the 18-knob defaults check (was 16 knobs; the rename from 1 bool to 1 enum + 2 new sweeper configs increases primary-knob count from 5 to 6, advanced unchanged at 11, hence 17... wait, the test count is 18 because compression-active-sweeper-interval is a separate config registration. Confirmed by the test passing.) Verified: make -j2 -C src SERVER_CFLAGS=-Werror # clean (BUILD_ZSTD=yes) make -C src distclean make -j2 -C src SERVER_CFLAGS=-Werror BUILD_ZSTD=no # clean ./runtest --single unit/type/compression # 13/13 pass Boot/runtime smoke: - --compression-master-switch compression --compression-threads 0 emits boot-time LL_WARNING. - CONFIG SET master compression -> decompression -> off logs the transitions correctly with the right "from" values. - INFO compression and COMPRESSION STATUS render the new fields. Not verified locally: - gtest unit tests (libgtest-dev not installed). CI validates. - clang-format-18. CI validates. * [S2.9 / C1] Verification-sweep follow-ups for PR-B vocabulary Three findings from a thorough re-sweep of the codebase for residual PR-B-vocabulary references: 1. compressionCommand() still routed `enable` / `disable` subcommands to a Phase-0 stub (return OK silently), and the HELP text listed `ENABLE, DISABLE, SWEEP, TRAIN` as future Phase-1 work. Per PR-B's design §4.5, the ENABLE/DISABLE aliases are NOT part of v1 (the 3-state enum doesn't map cleanly to enable/disable verbs). Removed the stub branch and rewrote the HELP text to reflect the v1 surface (STATUS / HELP land here; SWEEP FORCE / TRAIN / DICT * land in subsequent S2 PRs). 2. Three code comments cited "R2.1.5" when describing the "no active dictionary" state. Per the post-PR-B design that's actually R2.1.7 (master=compression but no active dict yet — the third state). Updated: - src/compression.c:1158 (compressionEnqueueCandidate's no-dict short-circuit) - src/compression.h:299 (header doc for the same) - src/compression_workers.c:619 (worker err>0 worker-policy sentinel comment) 3. .agents/planning/realtime-data-compression/idea-honing.md's PR-B header note said "Q1-Q16 hold up unchanged" but Q15 (testing strategy — §7.1 transparency-mode harness) describes a `--compression` flag whose server-config payload uses the old `--compression-enabled yes` etc. The flag's design intent is still valid, but the specific config names are superseded. Tightened the header note to flag Q15 explicitly and revise the unchanged-Qs list (Q1-Q4, Q6-Q14, Q16). Verified: make -j2 -C src SERVER_CFLAGS=-Werror # clean (BUILD_ZSTD=yes) ./runtest --single unit/type/compression # 13/13 pass * [S2.9 / C1] clang-format: collapse switch-case alignment + re-pad struct comments CI's clang-format-18 wants: 1. src/compression.c masterSwitchName(): switch-case 'return' values should NOT be column-aligned to the longest case label. The default LLVM 18 style collapses each 'case X: return Y;' onto a single line with one space. 2. src/server.h compressionState struct: the new compression_active_sweeper_interval field (35 chars) is longer than the prior longest field (compression_max_value_size at 26 chars), so the trailing-comment alignment column for ALL fields in this block needs to shift right (from column 39 to column 46). Also the continuation lines wrapping the multi-line comments need to align with the new comment column. Both findings reported by the failed clang-format-check job on PR valkey-io#29 (CI is the source of truth here — clang-format-18 isn't installed locally). Verified the build still passes after the formatting fix.
Remove trademarked wording on configuration layer.
Following changes for release notes:
pidfilein the template config file changed to/var/run/valkey_6379.pid