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 2022/02/25 00:22:53 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #7396: GEODE-9952: Support LSET Command
nonbinaryprogrammer opened a new pull request #7396:
URL: https://github.com/apache/geode/pull/7396
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
<!-- 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ezoerner edited a comment on pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
ezoerner edited a comment on pull request #7396:
URL: https://github.com/apache/geode/pull/7396#issuecomment-1051121828
Already discussed this in Slack, but LSET should be added to the list of supported commands in the docs file
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ezoerner commented on pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
ezoerner commented on pull request #7396:
URL: https://github.com/apache/geode/pull/7396#issuecomment-1051121828
Already discussed this in Slack, but the LSET should be added to the list of supported commands in the docs file
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r815158133
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -92,6 +93,19 @@ public int llen() {
return elementList.size();
}
+ /**
+ * @param region, the region to set on
+ * @param key, the key to set on
+ * @param index, the adjusted index to set (adjusted to a positive index)
+ * @param value, the value to set
+ * @return boolean if set
+ */
+ public boolean lset(Region<RedisKey, RedisData> region, RedisKey key, int index, byte[] value) {
+ byte[] delta = elementList.set(index, value);
+ storeChanges(region, key, new ReplaceByteArrayAtOffset(index, delta));
Review comment:
You don't want to ship the oldValue returned by "set" as the value in that delta. You can ignore the value returned by "set" (get rid of the "delta" variable) and pass "value" to the ReplaceByteArrayAtOffset constructor.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisList;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ Region<RedisKey, RedisData> region = context.getRegion();
+ List<byte[]> commandElements = command.getProcessedCommand();
+
+ RedisKey key = command.getKey();
+ if (!context.listLockedExecute(key, false, RedisData::exists)) {
+ return RedisResponse.error(ERROR_NO_SUCH_KEY);
+ }
+
+ long index = Coder.bytesToLong(commandElements.get(2));
+ byte[] value = commandElements.get(3);
+ long listSize = (long) context.listLockedExecute(key, false, RedisList::llen);
+
+ final int adjustedIndex = (int) (index >= 0 ? index : listSize + index);
Review comment:
I agree that this code needs move into the "lset" method. Also the LRANGE pull request is adding a new method that I think you can use to "adjust" your index.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7396:
URL: https://github.com/apache/geode/pull/7396#issuecomment-1051123365
This pull request **introduces 4 alerts** when merging b70d70040eff6cec8e405c6311649000708ec82d into 45cbe7f8df39704899b0305729749dc1cc9ffe89 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-4a3bb2e0059ea8590acda233019642b4d75d88e2)
**new alerts:**
* 4 for Spurious Javadoc @param tags
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r815016654
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java
##########
@@ -50,4 +50,9 @@ public long lpush(List<byte[]> elementsToAdd, Region<RedisKey, RedisData> region
public int llen() {
return 0;
}
+
+ @Override
+ public boolean lset(Region<RedisKey, RedisData> region, RedisKey key, int index, byte[] value) {
+ return false;
Review comment:
Instead I think you should do `throw new RedisException(ERROR_NO_SUCH_KEY)`
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisList;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ Region<RedisKey, RedisData> region = context.getRegion();
+ List<byte[]> commandElements = command.getProcessedCommand();
+
+ RedisKey key = command.getKey();
+ if (!context.listLockedExecute(key, false, RedisData::exists)) {
+ return RedisResponse.error(ERROR_NO_SUCH_KEY);
+ }
+
+ long index = Coder.bytesToLong(commandElements.get(2));
+ byte[] value = commandElements.get(3);
+ long listSize = (long) context.listLockedExecute(key, false, RedisList::llen);
Review comment:
There is a race here between this line and the actual call to `lset` on line 54. If we're`LSET`ting on the last element but the list is popped (and thus one element shorter) before getting to line 54 it will cause an IndexOutOfBounds exception. Prefer to do all of your checking within one 'locked' call - i.e. do this adjustment in the actual `lset` method.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisList;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ Region<RedisKey, RedisData> region = context.getRegion();
+ List<byte[]> commandElements = command.getProcessedCommand();
+
+ RedisKey key = command.getKey();
+ if (!context.listLockedExecute(key, false, RedisData::exists)) {
+ return RedisResponse.error(ERROR_NO_SUCH_KEY);
+ }
Review comment:
I think this can be removed in favor of my comment on `NullRedisList.lset`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ringles commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
ringles commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r822796462
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
Review comment:
Needs a test for operating on wrong key type - e.g. lset on a string or hash
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java
##########
@@ -16,6 +16,7 @@
package org.apache.geode.redis.internal.data;
+
Review comment:
Probably don't need this line, assume it was a minor oversight
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #7396:
URL: https://github.com/apache/geode/pull/7396#issuecomment-1055883968
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r826989730
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ RedisKey key = command.getKey();
+
+ return context.listLockedExecute(key, false, list -> {
Review comment:
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);
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r815194719
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisList;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ Region<RedisKey, RedisData> region = context.getRegion();
+ List<byte[]> commandElements = command.getProcessedCommand();
+
+ RedisKey key = command.getKey();
+ if (!context.listLockedExecute(key, false, RedisData::exists)) {
+ return RedisResponse.error(ERROR_NO_SUCH_KEY);
+ }
+
+ long index = Coder.bytesToLong(commandElements.get(2));
+ byte[] value = commandElements.get(3);
+ long listSize = (long) context.listLockedExecute(key, false, RedisList::llen);
+
+ final int adjustedIndex = (int) (index >= 0 ? index : listSize + index);
Review comment:
ok, I'll use that 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r818951509
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -92,6 +95,23 @@ public int llen() {
return elementList.size();
}
+ /**
+ * @param region the region to set on
+ * @param key the key to set on
+ * @param index the index specified by the user
+ * @param value the value to set
+ */
+ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, byte[] value) {
+ int listSize = elementList.size();
+ int adjustedIndex = index >= 0 ? index : listSize + index;
+ if (adjustedIndex > listSize - 1 || adjustedIndex < 0) {
+ throw new RedisException(ERROR_INDEX_OUT_OF_RANGE);
+ }
+
+ elementList.set(adjustedIndex, value);
Review comment:
you should use elementReplace here instead of elementList.set to give you the synchronization protection from a concurrent toData called by GII
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer merged pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #7396:
URL: https://github.com/apache/geode/pull/7396
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r827589901
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ RedisKey key = command.getKey();
+
+ return context.listLockedExecute(key, false, list -> {
Review comment:
We should still keep the `NumberFormatException` check but it just doesn't need to be inside the `listLockedExecute` call.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r817008483
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -33,6 +33,7 @@
public static final String ERROR_UNKNOWN_COMMAND =
"unknown command `%s`, with args beginning with: %s";
public static final String ERROR_OUT_OF_RANGE = "The number provided is out of range";
+ public static final String ERROR_INDEX_OUT_OF_RANGE = "index out of range";
Review comment:
After rebasing to pull in the changes from GEODE-10049, this string should be "ERR index out of range"
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
Review comment:
The order in which we handle error conditions in this class needs to be tweaked a bit to match what Redis does. Currently the two tests below pass with open source Redis but fail with this code:
```
@Test
public void lset_returnsNotAnIntegerError_givenKeyExistsAndIndexIsNotAValidInteger() {
jedis.lpush(KEY, initialValue);
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LSET, KEY, "notAnInteger", "newElement")).hasMessage("ERR " + ERROR_NOT_INTEGER);
}
@Test
public void lset_returnsNoSuchKeyError_givenKeyDoesNotExistAndIndexIsNotAValidInteger() {
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LSET, KEY, "notAnInteger", "newElement")).hasMessage("ERR " + ERROR_NO_SUCH_KEY);
}
```
We need to first check if the key exists, then check that the index is a valid integer (using a try/catch around `Coder.bytesToLong()`), all while holding the lock for the key. Changing the `executeCommand()` method to the following should fix these issues:
```
public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
List<byte[]> commandElements = command.getProcessedCommand();
RedisKey key = command.getKey();
return context.listLockedExecute(key, false, list -> {
if (!list.exists()) {
return RedisResponse.error(ERROR_NO_SUCH_KEY);
}
int index;
try {
index = narrowLongToInt(bytesToLong(commandElements.get(2)));
} catch (NumberFormatException ex) {
return RedisResponse.error(ERROR_NOT_INTEGER);
}
Region<RedisKey, RedisData> region = context.getRegion();
byte[] value = commandElements.get(3);
list.lset(region, key, index, value);
return RedisResponse.ok();
});
}
```
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String newValue = "newValue";
+ private JedisCluster jedis;
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ @Test
+ public void lset_errors_givenTooFewArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LSET, 3);
+ }
+
+ @Test
+ public void lset_onKeyThatDoesNotExist_returnsError_doesNotCreateKey() {
+ assertThatThrownBy(() -> jedis.lset(KEY, 1, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_NO_SUCH_KEY);
+ assertThat(jedis.exists(KEY)).isFalse();
+ }
+
+ @Test
+ public void lset_errors_withIndexOutOfRange() {
+ jedis.lpush(KEY, initialValue);
+ assertThatThrownBy(() -> jedis.lset(KEY, 10, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_INDEX_OUT_OF_RANGE);
+
+ assertThatThrownBy(() -> jedis.lset(KEY, -10, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_INDEX_OUT_OF_RANGE);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidPositiveIndex_withOneItemInList() {
+ jedis.lpush(KEY, initialValue);
+ assertThat(jedis.lset(KEY, 0, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidPositiveIndex_withMultipleItemsInList() {
+ for (int i = 4; i >= 0; i--) {
+ jedis.lpush(KEY, initialValue + i);
+ }
+
+ assertThat(jedis.lset(KEY, 2, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void lset_setsValue_givenValidNegativeIndex_withOneItemInList() {
+ jedis.lpush(KEY, initialValue);
+ assertThat(jedis.lset(KEY, -1, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidNegativeIndex_withMultipleItemsInList() {
+ for (int i = 4; i >= 0; i--) {
+ jedis.lpush(KEY, initialValue + i);
+ }
+
+ assertThat(jedis.lset(KEY, -2, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void lset_whenRunConcurrently_doesNotThrowRedisException() {
Review comment:
This test is not doing what you think it is, due to the way `ConcurrentLoopingThreads` handles exceptions. Replacing the assertions in the `catch` block with `assertThat(true).isFalse();` still results in the test passing.
A better test of correct concurrent behaviour for LSET would be to have a test that does an LSET concurrently with a multi-element LPUSH and confirms that either we set the element before the LPUSH happened, or we set it after, but not partway through:
```
@Test
public void lset_withConcurrentLpush_behavesCorrectly() {
String[] initialElements = {"1", "2", "3", "4", "5", "6"};
jedis.lpush(KEY, initialElements);
String[] elementsToAdd = {"a", "b", "c", "d", "e", "f"};
new ConcurrentLoopingThreads(1000,
i -> jedis.lpush(KEY, elementsToAdd),
i -> jedis.lset(KEY, 0, newValue)
).runWithAction(() -> {
assertThat(jedis.llen(KEY)).isEqualTo(initialElements.length + elementsToAdd.length);
assertThat(jedis).satisfiesAnyOf(
// LPUSH was applied first
jedisClient -> {
assertThat(jedisClient.lindex(KEY, 0)).isEqualTo(newValue);
assertThat(jedisClient.lindex(KEY, elementsToAdd.length)).isEqualTo(initialElements[initialElements.length - 1]);
},
// LSET was applied first
jedisClient -> {
assertThat(jedisClient.lindex(KEY, 0)).isEqualTo(elementsToAdd[elementsToAdd.length - 1]);
assertThat(jedisClient.lindex(KEY, elementsToAdd.length)).isEqualTo(newValue);
}
);
// Reset the list to its original contents
jedis.del(KEY);
jedis.lpush(KEY, initialElements);
});
}
```
We could also have a test for LSET with concurrent LPOP on a single-element list, to make sure that either LSET is applied first and we correctly set the element in the list then pop it, or LPOP is applied first and we get an ERROR_NO_SUCH_KEY response from LSET. If we get an ERROR_INDEX_OUT_OF_RANGE response, we know that LSET attempted to set the value on a nonexistent list, which shouldn't happen with correct locking:
```
@Test
public void lset_withConcurrentLpop_behavesCorrectly() {
jedis.lpush(KEY, initialValue);
AtomicReference<String> lpopResult = new AtomicReference<>();
AtomicReference<Throwable> lsetException = new AtomicReference<>();
new ConcurrentLoopingThreads(1000,
i -> lpopResult.set(jedis.lpop(KEY)),
i -> {
try {
jedis.lset(KEY, 0, newValue);
} catch (Exception ex) {
lsetException.set(ex);
}
}
).runWithAction(() -> {
assertThat(jedis.exists(KEY)).isFalse();
assertThat(lpopResult.get()).satisfiesAnyOf(
// LPOP was applied first
poppedValue -> {
assertThat(poppedValue).isEqualTo(initialValue);
assertThat(lsetException.get()).hasMessage("ERR " + ERROR_NO_SUCH_KEY);
},
// LSET was applied first
poppedValue -> {
assertThat(poppedValue).isEqualTo(newValue);
assertThat(lsetException.get()).isNull();
}
);
// Reset the list to its original contents and clear the atomic references
jedis.lpush(KEY, initialValue);
lpopResult.set(null);
lsetException.set(null);
});
}
```
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String newValue = "newValue";
+ private JedisCluster jedis;
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ @Test
+ public void lset_errors_givenTooFewArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LSET, 3);
+ }
+
+ @Test
+ public void lset_onKeyThatDoesNotExist_returnsError_doesNotCreateKey() {
+ assertThatThrownBy(() -> jedis.lset(KEY, 1, newValue))
+ .isInstanceOf(JedisDataException.class)
Review comment:
We shouldn't be asserting on the exception type returned here (or elsewhere in this class), as that's controlled by Jedis, not our code. if the Jedis client changes the exception they throw, this test will start failing despite the behaviour of the geode-for-redis server being correct.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -593,6 +593,14 @@ public void testLlen() {
runCommandAndAssertHitsAndMisses(LIST_KEY, k -> jedis.llen(k));
}
+ @Test
+ public void testLset() {
+ runCommandAndAssertNoStatUpdates(LIST_KEY, k -> {
+ jedis.lpush(k, "value");
+ jedis.lset(k, 1, "newvalue");
+ });
+ }
Review comment:
This can be simplified to:
```
runCommandAndAssertNoStatUpdates(LIST_KEY, k -> jedis.lset(k, 0, "newvalue"));
```
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java
##########
@@ -50,4 +53,9 @@ public long lpush(List<byte[]> elementsToAdd, Region<RedisKey, RedisData> region
public int llen() {
return 0;
}
+
+ @Override
+ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, byte[] value) {
+ throw new RedisException(ERROR_NO_SUCH_KEY);
+ }
Review comment:
If the approach described for `LSetExecutor` is used, this method is no longer needed.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements RedisIntegrationTest {
Review comment:
In addition to the two tests described in the comment on `LSetExecutor` could we also add a test for the behaviour when the key targeted by LSET is not a list?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r817727220
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
Review comment:
Somewhat contrary to this is to ask to keep as much of the code in the `RedisList.lset` method as possible. There's no technical reason, but it helps for consistency in reasoning about and understanding the responsibilities for the constructs we have defined. Ideally, the Executor should be doing as much validation as it can before locking and executing the actual change.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r827589901
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+
+import java.util.List;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.redis.internal.commands.Command;
+import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
+import org.apache.geode.redis.internal.commands.executor.RedisResponse;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ RedisKey key = command.getKey();
+
+ return context.listLockedExecute(key, false, list -> {
Review comment:
We should still keep the `NumberFormatException` check (and relevant test for that) but it just doesn't need to be inside the `listLockedExecute` call.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r815021164
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractLSetIntegrationTest implements RedisIntegrationTest {
Review comment:
Can you add a concurrency test that highlights the potential issue I mentioned in the `LSetExecutor`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7396: GEODE-9952: Support LSET Command
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7396:
URL: https://github.com/apache/geode/pull/7396#issuecomment-1050413776
This pull request **introduces 2 alerts** when merging f4155e44365f5a5a24f4239d11f1fd1ffedecdd3 into 1fc35af2186e0b51a10347a4b84d45e1d96136a6 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-14d16af3768289e9fa63ab97832b14275e750fa0)
**new alerts:**
* 2 for Spurious Javadoc @param tags
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org