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 XINFO, XGROUP, XACK, XTRIM, XCLAIM, and XDEL to commands.json #963

Merged
merged 3 commits into from Sep 28, 2018

Conversation

bobby
Copy link
Contributor

@bobby bobby commented Aug 28, 2018

Fixes #943

  • Needs analysis for time "complexity" entries
  • Summaries need reviewed, and XCLAIM summary needs added

- Needs analysis for time "complexity" entries
- Summaries need reviewed, and XCLAIM summary needs added
@bobby
Copy link
Contributor Author

bobby commented Aug 28, 2018

@antirez Please review. I don't understand Redis internals well enough to write the time "complexity" fields. Also, the "summary" fields might need some work, XCLAIM in particular I left blank due to a very long docstring in the source: https://github.com/antirez/redis/blob/unstable/src/t_stream.c#L1927

bobby added a commit to bobby/carmine that referenced this pull request Aug 28, 2018
@tirkarthi
Copy link

One another will be XINFO. More info at https://redis.io/topics/streams-intro#streams-observability. Sorry that I missed it in the list somehow.

127.0.0.1:6379> xinfo help
1) XINFO <subcommand> arg arg ... arg. Subcommands are:
2) CONSUMERS <key> <groupname>  -- Show consumer groups of group <groupname>.
3) GROUPS <key>                 -- Show the stream consumer groups.
4) STREAM <key>                 -- Show information about the stream.
5) HELP

Thanks

@bobby bobby changed the title Add XGROUP, XACK, XTRIM, XCLAIM, and XDEL to commands.json Add XINFO, XGROUP, XACK, XTRIM, XCLAIM, and XDEL to commands.json Sep 5, 2018
bobby added a commit to bobby/carmine that referenced this pull request Sep 5, 2018
- Includes XINFO after update of PR
@bobby
Copy link
Contributor Author

bobby commented Sep 5, 2018

@tirkarthi and @antirez I've added a first pass at XINFO. Any further guidance would be appreciated.

The absence of the Streams API from commands.json is blocking our production deployment (by way of taoensso/carmine#210) that uses the Streams API.

@itamarhaber
Copy link
Member

Personally, I think it is wonderful that the community is help with the docs and would like to see this merged sooner rather than later :)

That said, a client that depends on the documentation to work is odd IMO.

@tirkarthi
Copy link

Carmine uses https://raw.githubusercontent.com/antirez/redis-doc/master/commands.json similar to https://github.com/antirez/redis/blob/unstable/utils/generate-command-help.rb to generate all the functions with doc string and arities for the clojure redis client. If this is merged then Carmine can cut a release with the approved docs and arities. There is a PR to Carmine with commands generated from fork but it will be great to have it generated from redis repo and redis-cli can also be updated if necessary.

Thanks

@itamarhaber
Copy link
Member

I get the why and the reference to redis-cli is well in place - you need the "manifest" at build/generation time.

That said, the point I'm trying to raise is that I think a formal version manifest should be used. Formal in the sense that this project provides it, and in a decoupled manner from the docs (which are sometimes lagging/ahead). This could basically amount to an extra step, which dumps the output from COMMAND to some file, to Redis' version release process and won't be to hard to add to the release process or include in the cliens' deps.

In this way, clients, whether redis-cli or not, will be able rely on the interface published by that manifest file. However, until this gets resolved in/added to the core, the next best thing is for client authors to employ this idea by adding the extra step to their build process, or come up with better alternatives based on their needs and experience.

Disclaimer: not a client author :)

@antirez
Copy link
Contributor

antirez commented Sep 27, 2018

Hey dudes. First thanks a ton to @bobby I'm going to review and merge. About clients that rely on that, I think that the way to go is that the COMMAND command should be improved in order to provide all the information needed for clients to do this work without using a json file.

@antirez antirez merged commit d22f9da into redis:master Sep 28, 2018
@antirez
Copy link
Contributor

antirez commented Sep 28, 2018

PR merged, I'm going to edit summaries and time complexity. Thanks.

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

4 participants