You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/30 20:25:16 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #6235: GEODE-9087: redis docs for HSCAN command
nonbinaryprogrammer opened a new pull request #6235:
URL: https://github.com/apache/geode/pull/6235
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605970068
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw an exception.
Review comment:
I'm torn by the wording of this. We return an error type that follows Redis's [RESP protocol](https://redis.io/topics/protocol). The client library actually raises the exception when an error reply is received, not us. In this scenario, Redis seems to just say "an error is returned". The SCAN documentation is a bit different, but if you look at [DECRBY](https://redis.io/commands/DECRBY) they say the following:
```
An error is returned if the key contains a value of the wrong type or contains a string that cannot be
represented as integer. This operation is limited to 64 bit signed integers.
```
Also, maybe we could say `Redis accepts 64-bit signed integers for the cursor and COUNT parameters for the HSCAN command.` (since HSCAN also has the key, and MATCH parameters) and say we accept 32-bit integer values instead of `Integer values` to be a bit more specific (I see that specification in the following sentence, but thought it might be helpful to distinguish the value we say we accept in the previous sentence).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605970068
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw an exception.
Review comment:
I'm torn by the wording of this. We return an error type that follows Redis's [RESP protocol](https://redis.io/topics/protocol). The client library actually raises the exception when an error reply is received, not us. In this scenario, Redis seems to just say "an error is returned". The SCAN documentation is a bit different, but if you look at [DECRBY](https://redis.io/commands/DECRBY) they say the following:
```
An error is returned if the key contains a value of the wrong type or contains a string that can not be represented as integer. This operation is limited to 64 bit signed integers.
```
Also, maybe we could say `Redis accepts 64-bit signed integers for the cursor and COUNT parameters for the HSCAN command.` and say we accept 32-bit integer values instead of `Integer values` to be a bit more specific (I see that specification in the following sentence, but thought it might be helpful to distinguish the value we say we accept in the previous sentence).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r607205527
##########
File path: geode-docs/tools_modules/geode_apis_compatible_with_redis.html.md.erb
##########
@@ -68,6 +68,7 @@ The <%=vars.product_name%> APIs compatible with Redis currently supports the fol
**Note**: These commands are supported for Redis 5.
**Note**: SLOWLOG is implemented as a NoOp for compatibility with some monitoring tools.
+**Note**: Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Parameters out of the range of the 32-bit integer type will throw an exception.
Review comment:
I definitely agree with that but the current format doesn't make that very easy. [this PR](https://github.com/apache/geode/pull/6239/) changes the format of this list, which will allow an inline note. I will make this change when I rebase that command on top of this (once it is merged)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605970068
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw an exception.
Review comment:
I'm torn by the wording of this. We return an error type that follows Redis's [RESP protocol](https://redis.io/topics/protocol). The client library actually raises the exception when an error reply is received, not us. In this scenario, Redis seems to just say "an error is returned". The SCAN documentation is a bit different, but if you look at [DECRBY](https://redis.io/commands/DECRBY) they say the following:
```
An error is returned if the key contains a value of the wrong type or contains a string that can not be
represented as integer. This operation is limited to 64 bit signed integers.
```
Also, maybe we could say `Redis accepts 64-bit signed integers for the cursor and COUNT parameters for the HSCAN command.` and say we accept 32-bit integer values instead of `Integer values` to be a bit more specific (I see that specification in the following sentence, but thought it might be helpful to distinguish the value we say we accept in the previous sentence).
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw an exception.
Review comment:
I'm torn by the wording of this. We return an error type that follows Redis's [RESP protocol](https://redis.io/topics/protocol). The client library actually raises the exception when an error reply is received, not us. In this scenario, Redis seems to just say "an error is returned". The SCAN documentation is a bit different, but if you look at [DECRBY](https://redis.io/commands/DECRBY) they say the following:
```
An error is returned if the key contains a value of the wrong type or contains a string that cannot be
represented as integer. This operation is limited to 64 bit signed integers.
```
Also, maybe we could say `Redis accepts 64-bit signed integers for the cursor and COUNT parameters for the HSCAN command.` and say we accept 32-bit integer values instead of `Integer values` to be a bit more specific (I see that specification in the following sentence, but thought it might be helpful to distinguish the value we say we accept in the previous sentence).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605971134
##########
File path: geode-docs/tools_modules/geode_apis_compatible_with_redis.html.md.erb
##########
@@ -68,6 +68,7 @@ The <%=vars.product_name%> APIs compatible with Redis currently supports the fol
**Note**: These commands are supported for Redis 5.
**Note**: SLOWLOG is implemented as a NoOp for compatibility with some monitoring tools.
+**Note**: Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Parameters out of the range of the 32-bit integer type will throw an exception.
Review comment:
Forgot to mention, @karensmolermiller previously made a comment in [this PR](https://github.com/apache/geode/pull/6149) regarding the command notes:
```
If there will be more notes about specific commands, I'd like to see the notes placed next to the
command within the lists. Maybe add the note in parenthesis? I believe that documentation readers are
more likely to see the note if it is with the command, as I believe that readers will be likely to scan
this page for the commands that they use. I don't think they'll be starting at the top to do a detailed
reading.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r608025541
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts 64-bit signed integer for the cursor and COUNT parameters be represented as an integer for the HSCAN command. This operation is limited to 32-bit integer values.
Review comment:
wow I completely mangled that text. idk what happened
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r607225328
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts 64-bit signed integer for the cursor and COUNT parameters be represented as an integer for the HSCAN command. This operation is limited to 32-bit integer values.
Review comment:
Was this meant to be something like:
```
Redis accepts 64-bit signed integers for the HSCAN cursor and COUNT parameters. The Geode APIs compatible with Redis accept 32-bit integers for these parameters.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r604976313
##########
File path: geode-docs/tools_modules/geode_apis_compatible_with_redis.html.md.erb
##########
@@ -68,6 +68,7 @@ The <%=vars.product_name%> APIs compatible with Redis currently supports the fol
**Note**: These commands are supported for Redis 5.
**Note**: SLOWLOG is implemented as a NoOp for compatibility with some monitoring tools.
+**Note**: Redis accepts Longs as the cursor index for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Cursors out of the range of the 32-bit integer type will throw a NumberFormatException.
Review comment:
I don't think we need to mention the `NumberFormatException`. The user never sees that because we catch it and return a Redis error response. Maybe we could say something like:
```
An error containing the message `invalid cursor` is returned if the cursor value is out of range of the 32-bit integer type.
```
By the way, the same is also true for the HSCAN COUNT parameter (Redis accepts up to a 64-bit integer, but we only accept up to a 32-bit integer)... I'm not sure if we want to add that difference as part of this PR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605971134
##########
File path: geode-docs/tools_modules/geode_apis_compatible_with_redis.html.md.erb
##########
@@ -68,6 +68,7 @@ The <%=vars.product_name%> APIs compatible with Redis currently supports the fol
**Note**: These commands are supported for Redis 5.
**Note**: SLOWLOG is implemented as a NoOp for compatibility with some monitoring tools.
+**Note**: Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Parameters out of the range of the 32-bit integer type will throw an exception.
Review comment:
Forgot to mention, @karensmolermiller previously made a comment in [this PR](https://github.com/apache/geode/pull/6149) regarding the command notes:
```
If there will be more notes about specific commands, I'd like to see the notes placed next to the command within the lists. Maybe add the note in parenthesis? I believe that documentation readers are more likely to see the note if it is with the command, as I believe that readers will be likely to scan this page for the commands that they use. I don't think they'll be starting at the top to do a detailed reading.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on pull request #6235:
URL: https://github.com/apache/geode/pull/6235#issuecomment-815142756
> @nonbinaryprogrammer LGTM +1
> I should alert you to some massive changes to the README that I submitted for PR 9068 (#6239). LMK if I can help with resolving any merge conflicts. (Better yet, merge first and let the submitters of the massive changes deal with them.)
thanks @davebarnes97, Hutch and I are in communication about it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer merged pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #6235:
URL: https://github.com/apache/geode/pull/6235
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r608806510
##########
File path: geode-docs/tools_modules/geode_apis_compatible_with_redis.html.md.erb
##########
@@ -68,6 +68,7 @@ The <%=vars.product_name%> APIs compatible with Redis currently supports the fol
**Note**: These commands are supported for Redis 5.
**Note**: SLOWLOG is implemented as a NoOp for compatibility with some monitoring tools.
+**Note**: Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept 32-bit integer values. Parameters out of the range of this range will throw an exception.
Review comment:
Could we change this note to match the note in the docs?
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts 64-bit signed integers for the HSCAN cursor and COUNT parameters. The Geode APIs compatible with Redis is limited to 32-bit integer values for these parameters.
Review comment:
is -> are
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r605970068
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the parameters for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw an exception.
Review comment:
I'm torn by the wording of this. We return an error type that follows Redis's [RESP protocol](https://redis.io/topics/protocol). The client library actually raises the exception when an error reply is received, not us. In this scenario, Redis seems to just say "an error is returned". The SCAN documentation is a bit different, but if you look at [DECRBY](https://redis.io/commands/DECRBY) they say the following:
```
An error is returned if the key contains a value of the wrong type or contains a string that cannot be
represented as integer. This operation is limited to 64 bit signed integers.
```
Also, maybe we could say `Redis accepts 64-bit signed integers for the cursor and COUNT parameters for the HSCAN command.` (since HSCAN also has the key and MATCH parameters) and say we accept 32-bit integer values instead of `Integer values` to be a bit more specific (I see that specification in the following sentence, but thought it might be helpful to distinguish the value we say we accept in the previous sentence).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r607225328
##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts 64-bit signed integer for the cursor and COUNT parameters be represented as an integer for the HSCAN command. This operation is limited to 32-bit integer values.
Review comment:
Was this meant to be something like:
```
Redis accepts 64-bit signed integers for the HSCAN cursor and COUNT parameters. The Geode APIs compatible with Redis only accept 32-bit integers for these parameters.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6235: GEODE-9087: redis docs for HSCAN command
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6235:
URL: https://github.com/apache/geode/pull/6235#discussion_r604978014
##########
File path: geode-redis/README.md
##########
@@ -329,9 +329,10 @@ start server \
| | | ZREVRANK |
| | | ZSCAN |
| | | ZSCORE |
-| | | ZUNIONSTORE | |
+| | | ZUNIONSTORE |
**NOTES:**
[1] - UNLINK is implemented as a synonym to DEL and does not unlink asynchronously.
[2] - SLOWLOG is implemented as a NoOp.
+[3] - Redis accepts Longs as the cursor index for the HSCAN command. The Geode APIs compatible with Redis only accept Integer values. Values out of the range of the 32-bit integer type will throw a NumberFormatException with a message containing "invalid cursor".
Review comment:
Same comment as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org