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.

Sign up for GitHub

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

Jump to bottom

GEODE-9952: Support LSET Command #7396

Merged
merged 14 commits into from
Mar 18, 2022
Merged

GEODE-9952: Support LSET Command #7396

merged 14 commits into from
Mar 18, 2022

Conversation

nonbinaryprogrammer
Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer commented Feb 25, 2022 •
edited

Implements the LSET command as described by the Redis documentation.

LSET key index newValue
ex. LSET key 1 "newValue"

The LSET command sets the list element at the given index to the
specified new value.

If the value is negative, it wraps around.

If the index, after being adjusted for negative values, is out of
bounds, the RedisResponse will be an index out of bounds error.

If the key does not exist then it will not be created and a key does not
exist error will be returned.

The command may return:

  • OK
  • ERR index out of bounds
  • ERR no such key

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?

@nonbinaryprogrammer nonbinaryprogrammer added the redis Issues related to the geode-for-redis module label Feb 25, 2022
@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request introduces 2 alerts when merging f4155e4 into 1fc35af - view on LGTM.com

new alerts:

  • 2 for Spurious Javadoc @param tags

@ezoerner
Copy link
Contributor

ezoerner commented Feb 25, 2022 •
edited

Already discussed this in Slack, but LSET should be added to the list of supported commands in the docs file

@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request introduces 4 alerts when merging b70d700 into 45cbe7f - view on LGTM.com

new alerts:

  • 4 for Spurious Javadoc @param tags

DonalEvans
DonalEvans requested changes Mar 1, 2022
View reviewed changes
.../apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java Outdated Show resolved Hide resolved
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java Outdated Show resolved Hide resolved
...redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java Show resolved Hide resolved
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java Outdated Show resolved Hide resolved
...java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java Outdated Show resolved Hide resolved
...java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java Show resolved Hide resolved
...java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java Outdated Show resolved Hide resolved
@DonalEvans
Copy link
Contributor

In addition to the changes requested above, we should add a DUnit test for LSET, specifically one where we call LSET then crash the primary, then assert that the contents of the secondary matches what we expect after failing over, to make sure that deltas are being propagated and applied correctly.

ringles
ringles requested changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@ringles ringles left a comment

Choose a reason for hiding this comment

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

Couple minor bits, otherwise seems legit

...java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java Show resolved Hide resolved
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java Outdated Show resolved Hide resolved
jdeppe-pivotal
jdeppe-pivotal requested changes Mar 15, 2022
View reviewed changes
...redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
List<byte[]> commandElements = command.getProcessedCommand();
RedisKey key = command.getKey();

return context.listLockedExecute(key, false, list -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought I had commented here but now realize I hadn't. A few days ago there was a discussion (but I think you were out) that we don't need to support multiple command error conditions in the same 'order' that redis does - i.e. wrong key AND wrong index (say an invalid number) produces errors in the same order as redis. (So we can remove tests that check for this). This will avoid the need for checking existence here and then you can also pull the number check out of this block. Ultimately it simplifies this code and makes it more consistent with our other code. You'd end up with what we usually have, namely something like:

return context.listLockedExecute(key, false, list -> list.lset(region, key, index, value);

Copy link
Contributor

Choose a reason for hiding this comment

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

We should still keep the NumberFormatException check (and relevant test for that) but it just doesn't need to be inside the listLockedExecute call.

jdeppe-pivotal
jdeppe-pivotal requested changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@jdeppe-pivotal jdeppe-pivotal left a comment

Choose a reason for hiding this comment

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

We still need to keep the NumberFormatException in LSetExecutor.

@nonbinaryprogrammer
GEODE-9952: Support LSET Command
21783fe 
Implements the LSET command as described by the Redis documentation.

LSET key index newValue
ex. LSET key 1 "newValue"

The LSET command sets the list element at the given index to the
specified new value.

If the value is negative, it wraps around.

If the index, after being adjusted for negative values, is out of
bounds, the RedisResponse will be an index out of bounds error.

If the key does not exist then it will not be created and a key does not
exist error will be returned.

The command may return:
- OK
- ERR index out of bounds
- ERR no such key
@nonbinaryprogrammer
add DENYOOM in RedisCommandType
b862f21
@nonbinaryprogrammer
fix LGTM warnings
7a80448
@nonbinaryprogrammer
move checking of given index into lset(). add a concurrency test. upd…
86e10e2 
…ate docs
@nonbinaryprogrammer
use the new value as the delta instead of the old one
bc08c9e
@nonbinaryprogrammer
return null from lambda instead of returning boolean from lset
fb16cf6
@nonbinaryprogrammer
PR comments
607f4ea
@nonbinaryprogrammer
fix issue from merge and remove excess code from NullRedisList
57b3829
@nonbinaryprogrammer
run spotless after rebase
3793603
@nonbinaryprogrammer
add test for wrong key type and remove extra line
e8f004b
@nonbinaryprogrammer
simplify error handling for lset and remove tests that enforce error …
7d33356 
…order
@nonbinaryprogrammer
fix test issue
e61a633
@nonbinaryprogrammer
add back handling of NumberFormatException
7280af7
@nonbinaryprogrammer
remove extra import after rebase
f3f02e1
jdeppe-pivotal
jdeppe-pivotal approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@jdeppe-pivotal jdeppe-pivotal left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for your changes.

ringles
ringles approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@ringles ringles left a comment

Choose a reason for hiding this comment

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

Looks good!

@nonbinaryprogrammer nonbinaryprogrammer merged commit 669149d into apache:develop Mar 18, 2022
@nonbinaryprogrammer nonbinaryprogrammer deleted the GEODE-9952-LSET branch March 18, 2022 17:02
Kris-10-0 pushed a commit to Kris-10-0/geode that referenced this pull request Mar 18, 2022
@nonbinaryprogrammer @Kris-10-0
GEODE-9952: Support LSET Command (apache#7396)
23603fb 
Implements the LSET command as described by the Redis documentation.

LSET key index newValue
ex. LSET key 1 "newValue"

The LSET command sets the list element at the given index to the
specified new value.

If the value is negative, it wraps around.

If the index, after being adjusted for negative values, is out of
bounds, the RedisResponse will be an index out of bounds error.

If the key does not exist then it will not be created and a key does not
exist error will be returned.

The command may return:
- OK
- ERR index out of bounds
- ERR no such key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdeppe-pivotal jdeppe-pivotal jdeppe-pivotal approved these changes

@dschneider-pivotal dschneider-pivotal dschneider-pivotal approved these changes

@DonalEvans DonalEvans DonalEvans approved these changes

@ringles ringles ringles approved these changes

@upthewaterspout upthewaterspout Awaiting requested review from upthewaterspout

@davebarnes97 davebarnes97 Awaiting requested review from davebarnes97

Assignees
No one assigned
Labels
redis Issues related to the geode-for-redis module
Projects
None yet
Milestone
No milestone
6 participants
@nonbinaryprogrammer @ezoerner @DonalEvans @jdeppe-pivotal @dschneider-pivotal @ringles