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

Add information about server domain object for LUA #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

madolson
Copy link
Member

@madolson madolson commented May 4, 2024

Documents the behavior for the new server top level domain object and the new Lua fields. Also updates all of the examples to use the server object. Also includes some cleanup to references to "Valkey serialization protocol" which doesn't exist.

There will be some minimal conflicts with #22, as it attempts to remove the versioning references which this file also does for the files that were touched.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
@madolson madolson requested a review from zuiderkwast May 4, 2024 20:15
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.

Good. I just think we should keep version information. Let's discuss that.


* Since version: 7.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the version information is important. I disagree with the PR deleting all this.

Users may write Lua scripts that are need to run on a range of versions of Valkey and Redis OSS, so depending on which versions they need to support, they need to know what they can use. Let's say they're writing some open source lib or tool or so.

For things that aren't yet released, it's even more important, but this one is new in Valkey 7.2.5, right?

Somewhere in the docs, we can write that versions < 7.2.5 always refer to Redis OSS and >= 7.2.5 refer to Valkey versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how strongly I feel about that. There are some very old versions of Redis that have weird behavior, are we going to document all of that behavior? Is there a single issue where we are discussing this? Should we add it to the contributor meeting we have on Monday?

Users may write Lua scripts that are need to run on a range of versions of Valkey and Redis OSS, so depending on which versions they need to support, they need to know what they can use. Let's say they're writing some open source lib or tool or so.

I don't think it's our job to detail all of the historical behavior changes in Redis. I am documenting all of the "breaking" changes, and intend to write a single page including everything that could break if you are moving from Redis to Valkey.

Copy link
Contributor

Choose a reason for hiding this comment

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

Old versions, please delete. The only ones (IMO) that may be of interest are the still supported versions, i.e. 6.2, 7.0 and 7.2.

It's been discussed in random PRs. There's no single place AFAIK.

I don't think it's our job to detail all of the historical behavior changes in Redis.

That's one way to look at it.

Here's another: If you consider Valkey the continuation of Redis OSS that just changed its name, then it does make sense to keep historical info. We're still the same project, so version history is as useless or useful as it would be without a name change.

intend to write a single page including everything that could break if you are moving from Redis to Valkey.

Good, but I think that's release notes material. That's what people should review when updating. If people don't read the release notes, then maybe they're not accessible enough? We should link to them on the download page next to each version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Old versions, please delete. The only ones (IMO) that may be of interest are the still supported versions, i.e. 6.2, 7.0 and 7.2.

Ok. I'd commit to keeping information about versions 7-6.2 for historical purposes. We can document we drop references from versions of Redis before that point.

Good, but I think that's release notes material. That's what people should review when updating. If people don't read the release notes, then maybe they're not accessible enough? We should link to them on the download page next to each version.

I was going to do it as a one-off to build trust for folks to migrate. I agree generally that is for release notes, but it will be confusing initially as folks move from Redis -> valkey.

Comment on lines +521 to +523
### <a name="server.redis_version"></a> `server.REDIS_VERSION`

* Available in scripts: yes
Copy link
Contributor

Choose a reason for hiding this comment

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

If called as redis.REDIS_VERSION, it came in 7.0.0.

Suggested change
### <a name="server.redis_version"></a> `server.REDIS_VERSION`
* Available in scripts: yes
### <a name="server.redis_version"></a> `server.REDIS_VERSION`
* Since version: 7.0.0
* Available in scripts: yes

topics/lua-api.md Show resolved Hide resolved
Comment on lines +88 to +90
**Note:**
For compatibility with Redis, Valkey also exposes a _redis_ top level object, that exposes the exact same set of APIs as the _server_ object.
Valkey does not intend to drop compatibility for this _redis_ API, but it is recommended to use the _server_ object for newly developed scripts.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep the "Since" information, we can explain here that the server object exists since 7.2.5. Earlier versions (Redis OSS) only support the redis object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants