Git Product home page Git Product logo

Comments (25)

zuiderkwast avatar zuiderkwast commented on June 4, 2024 2

I don't really see why we need different release candidates. The actual releases are more important.

I think we can do Lua server object as an alias to redis at any time. Separate PR though.

I think we can postpone valkeymodule.h indefinitely. Can we just keep redismodule.h with existing APIs until we have some new APIs we want to add?

The 7.2.4 is compatibility only. We can add alias but we can't break anything. This is based on an existing tag and stuff are backported from unstable. The next release (minor or major) is created from unstable. There are a lot of unreleased stuff in unstable that we got when forking. Perhaps we should consider another 7.x release with those things?

from valkey.

zuiderkwast avatar zuiderkwast commented on June 4, 2024 1

Very good. Agree on most of it.

Binaries: For the full compatibility release, it's best to keep the redis-server and redis-cli, at least as symlinks created by make. It seems to be fair use.

For INFO and RDB fields, let's call the new field server_name instead of server_type. Name and version go together.

For Lua, let's add server.call(), but let's also keep redis.call() indefinitely, for compatibility. It is a very important part of the command API. We don't want websites to break when the sysadmin switches to placeholderkv.

For logging and error messages, let's keep them for now but let's change them in the next major release. Let's assume they don't have to be identical to claim compatibility. They are not documented in detail.

from valkey.

madolson avatar madolson commented on June 4, 2024

For some components, like check-aof, we might have a problem where they expect the name to look a certain way. We should probably rewrite them so that they are agnostic of what the name of the file is.

from valkey.

parthpatel avatar parthpatel commented on June 4, 2024

Couple of comments.

  1. We should consistently use placeholder vs placeholderkv across these.
placeholderkv-server
placeholderkv-cli
placeholder-benchmark
placeholder-check-aof
placeholder-check-rdb
placeholder-sentinel
  1. There are 3 flows that encounter "redis" names as per my understanding:
    a) end user facing naming - Clones repo, reads README, builds and runs the server/client Or installs via rpm. They may modify the flags in Makefile or use utils scripts. In Makefile, REDIS_FLAGS was the only reference. In utils directory, I see scripts like redis-sha1 or redis-copy etc which are searching for redis executable. Do we want to change them?
    b) developer facing naming - reads CONTRIBUTING file and opens a pull request OR writes a module to publish against the repo OR connects to Redis endpoint using one of the many supported clients (info and redis.call commands look appropriate changes, not so sure about RDB file changes). I would say this is a very wide scoped item. We should not touch this for 7.2 just yet.
    c) legal naming in license etc. - this is a given so not commenting on it.

  2. For RDB format change, are we sure that any custom parsers always know to skip unrecognized aux fields? Or do we want to simply break the compatibility there to remove trademark.

  3. I agree with logging remaining same. For Module and LUA scripting calls, how do we force folks to migrate to new calls in future? Also, which version would that happen in? On that note, what will be the changes in next major version assuming that we only do these changes right now?

  4. When I grep the codebase for redis, I see a lot of references in hiredis library. Is this client only internally consumed or do we want to rename it?

from valkey.

madolson avatar madolson commented on June 4, 2024

@placeholderkv/core-team Will you also comment here when you get a chance

from valkey.

hwware avatar hwware commented on June 4, 2024

I have 2 notes:

  1. If we could add new aux field in later version, so in RC1 we could keep the maximum compatibility with Redis 7.2
  2. some commands, such as LOLWUT, we need rewrite. Need to investigate more commands like this.

from valkey.

daniel-house avatar daniel-house commented on June 4, 2024

It seems to me that when "redis" is part of a protocol/api, it is not being used as a trademark. When it appears in documentation or on a web-site refering to a product, then it is being used as a trademark. In this interpretation (note, I am not a lawyer) the response from HELLO or the names of module-api entry points like REDISMODULE_*, the contents and extension of rdb files, etc, are not trademarks and should not require changing. These are the things we are told we can use from 7.2.4 and are essential to being able to use 7.2.4. They are also the things that are required for drop-in compatibility. On the other hand, the names of the server or cli executables do look like trademarks. Except for statements like "the Redis protocol specifies that ...", nearly any mention of Redis in the documentation is probably a trademark violation. The Redis protocol has become a de-facto industry standard and it seems to me that RedisLabs has lost control of the "Redis protocol" trademark via common usage in the same way as the words "kleenex" and "vise grips".

from valkey.

PingXie avatar PingXie commented on June 4, 2024

for 7.2.4 RC1

We will introduce 2 new info fields

I would suggest we move these two new info fields (server_version/server_name) to RC2, where we will close on the compat strategy (your strict/loose/none proposal). I am concerned about some client apps counting lines in the info output. let's be consistent with our compat story in RC1, which is no breaking change/same look-n-feel.

In RDB format, we will add two new aux fields

I am less concerned with these two fields but from the consistency's perspective, I think we can avoid one-offs and wait until RC2 to resolve all compat issues holistically. Then we just need to set a timeline to drop the compat, which hopefully will give enough time for 80+% of clients to add the native support. thoughts?

from valkey.

madolson avatar madolson commented on June 4, 2024

I think we can avoid one-offs and wait until RC2 to resolve all compat issues holistically.

This seems like a larger scope than we need. I think the original focus on compatibility just should be so we can remove all references to Redis. Additional work like deprecating master/slave, or other major changes, I think we should consider to the first major release.

from valkey.

PingXie avatar PingXie commented on June 4, 2024

Let me be explicit and make sure we are on the same page

1.We will build with the following symlink that map to corresponding redis artifacts.

Agreed. Here is my understanding of the next level actions:

  • valkey-* will be the default name and we create redis-* symlinks to their valkey-* counterpart during make install
  • OS/container image distributors are responsible for making sure the redis-* links are present

2.We will introduce 2 new info fields.

This is a NO for me

  1. In RDB format, we will add two new aux fields

Sure. I am OK with adding them in 7.2. Like I mentioned earlier, I am not concerned about compat with these two new aux fields. I see your point of making smaller changes along the way. Make sense to me.

4.Logging and other misc wording

Agreed with no change in all 7.2 releases

7.2 RC2

Agreed. Dual-system works for me (server.call() vs redis.call(), and RedisModuleAPI vs ValkeyModuleAPI)

from valkey.

hwware avatar hwware commented on June 4, 2024

Some active prs already invloves the redis keyword changes,
I think we need to discuss if we need to replace all of these in RC1 as the following categories?

  • Filename
  • Variable name, Function name
  • Logs ------------- out of in RC1 plan
  • Config parameter ------------- out of in RC1 plan
  • Test file
  • CI file
  • Json file
  • RedisModule related
  • RESP client return message ------------- out of in RC1 plan
  • Comments
  • Others? Free to add.

@valkey-io/core-team

from valkey.

PingXie avatar PingXie commented on June 4, 2024

I think the following should be out of RC1. Others look great to me (file names will be symlinked)

  • ABI/API compat (RedisModule/Lua) // RC2
  • Logs and client output // first major release?

from valkey.

hwware avatar hwware commented on June 4, 2024

Based on @PingXie words, I just draft the RC plan:

  1. RC1: Source code, comments, and any words except those on the "user face side" will be changed. "User face side" for example, the log, client message, and valkey(redis) config parameters will not be changed.

  2. RC2: Here are some items I suggest adding:

    • Server_version:7.2.4 and server_name:valkey are two new information fields
    • Deprecate master/slave, use primary/replica, and update all related information
    • (To be discussed) Whether to update the LOG message or not
    • The User client message should be updated or added with some fields
  3. RC3:

    • Compatibility with LUA
    • Compatibility with module APIs

from valkey.

PingXie avatar PingXie commented on June 4, 2024

I think the two RC3 items can be pulled into RC2 since we will be aliasing them.

All of your RC2 proposals though sound like a major release thing. For instance, switching to primary and replica in CLUSTER NODES will definitely break every cluster client out there. The two new fields (server_version and server_name) and client output updates (like returning a different string in the server field) are of median risk IMO. Logging could have impact too.

from valkey.

madolson avatar madolson commented on June 4, 2024

[adding two new info fields] is a NO for me

I need to understand why this is a blocker. The reason given (counting number of the lines) doesn't seem like a real use case. What is our versioning strategy moving forward?

from valkey.

PingXie avatar PingXie commented on June 4, 2024

[adding two new info fields] is a NO for me

I need to understand why this is a blocker. The reason given (counting number of the lines) doesn't seem like a real use case.

I need to clarify my statement a bit. I meant to say not adding these two fields in RC1 as, depending on where we add them, it has the risk to confuse some clients (I don't have a concrete example). If the new fields are added to the end of the INFO output, I think that is a lot safer but then it is ideal when we get to theh first major release - see below

What is our versioning strategy moving forward?

The first major release would be the ideal place to swap these two Redis strings out. Customers who want the compat can choose to keep the Redis strings. Then at some point, we drop the compat support, and no one gets to see any Redis strings afterwards.

from valkey.

madolson avatar madolson commented on June 4, 2024

Oh, I see, you are concerned because people might be explicitly looking for the 2nd and 3rd line. Would it alleviate the concern if we placed them at the bottom of the server info field?

from valkey.

PingXie avatar PingXie commented on June 4, 2024

Oh, I see, you are concerned because people might be explicitly looking for the 2nd and 3rd line. Would it alleviate the concern if we placed them at the bottom of the server info field?

Yes but that is not the ideal place for these two fields. Ideally, we would like to swap out the two Redis fields with the new fields. Once we place these two new fields at the end of the output, I worry that they will get stuck there.

from valkey.

zuiderkwast avatar zuiderkwast commented on June 4, 2024

Pidfile.

Default is "/var/run/redis.pid". Keep or change?

In the config file, it's suggested to configure it to "/var/run/redis_6379.pid". This we can change to valkey_6379 right? It's just a template config file to use as inspiration.

from valkey.

daniel-house avatar daniel-house commented on June 4, 2024

I find the template config, .../src/redis.conf, to be the best (sometimes only) documentation for the configuration parameters. It is supposed to also agree with the default settings that you get when you don't specify a config file. Changing its name to valkey.conf is almost certain to break someone's deployment scripts.

from valkey.

madolson avatar madolson commented on June 4, 2024

I find the template config, .../src/redis.conf, to be the best (sometimes only) documentation for the configuration parameters. It is supposed to also agree with the default settings that you get when you don't specify a config file. Changing its name to valkey.conf is almost certain to break someone's deployment scripts.

This is the thing I'm actually struggling to believe is true. I agree it's the best documentation for redis, but I think people will be willing to transition to valkey.conf. One thing I can say, we do plan on doing release candidates, so if someone is using it, hopefully it will come up?

from valkey.

PingXie avatar PingXie commented on June 4, 2024

Yeah I think we should get the feedback from the release candidate.

I agree with "redis.conf being the best documentation for the config". I see little/no operational value in redis.conf as I have not encountered a case where redis.conf is used as-is in production. On the other hand, I can imagine an admin/operator developing a workflow that depends on the pid file path, though I haven't seen one either.

from valkey.

daniel-house avatar daniel-house commented on June 4, 2024

Oh, I don't object to renaming it. In fact, I think it is disgusting to find the best documentation is in an example config file. I just fear losing such vital information or making it harder to find than it already is.

from valkey.

madolson avatar madolson commented on June 4, 2024

Based on offline conversations, updated the current RC1 plan with the following changes:

  1. server_version -> valkey_version in server info. Since we would like to advertise specific compatibility, we are making the version specific to valkey. servername will remain as an optional indicator, and other valkey compatible stores might choose to advertise something else.
  2. We dropped redis-ver from the API. This isn't related to API compatibility, but we didn't want to "fake" that valkey was creating an rdb from a Redis version.
  3. Renamed server-ver -> valkey_version in rdb info. Same as point one, we want to explicitly indicate this was created by a valkey server.

from valkey.

zuiderkwast avatar zuiderkwast commented on June 4, 2024

Even though we've decided not to change logging entries, we may want to make some exceptions to a few of them, apart from the logo? Like "Redis is starting", "Redis is shutting down, bye bye" etc. I feel we need to decide where draw the line.

The rest of them can be guarded by a compat switch that we add in the next major version.

from valkey.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.