dschneider-pivotal commented on a change in pull request #7392: URL: https://github.com/apache/geode/pull/7392#discussion_r815132743
########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LInsertExecutor.java ########## @@ -0,0 +1,55 @@ +/* + * 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.RedisConstants; +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 LInsertExecutor implements CommandExecutor { + + @Override + public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) { + List<byte[]> commandElements = command.getProcessedCommand(); + Region<RedisKey, RedisData> region = context.getRegion(); + RedisKey key = command.getKey(); + + String direction = Coder.bytesToString(commandElements.get(2)); + boolean before; + byte[] referenceElement = commandElements.get(3); + byte[] elementToInsert = commandElements.get(4); + + if (direction.equalsIgnoreCase("before")) { + before = true; + } else if (direction.equalsIgnoreCase("after")) { + before = false; + } else { + return RedisResponse.error(RedisConstants.ERROR_SYNTAX); + } + + long numEntries = context.listLockedExecute(key, false, Review comment: I noticed in another PR that for index we use a 32-bit "int" on lists. I also see that index on this PR's applyInsertByteArrayDelta is an "int". So why is "numEntries" a long? If the max size of a list is 2G then this can return an int. But if the list can be bigger than 2G then our indexes need to also be "long". ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -135,6 +161,27 @@ public int getDSFID() { return REDIS_LIST_ID; } + public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement, + boolean before) { + for (int i = 0; i < elementList.size(); i++) { Review comment: I think you definitely want to change this to iterate the elements instead of using elementList.get(i). Since elementList is a linked list, every time you call it it has to walk the linked list again to find element 'i'. If you instead just do "for (byte[] element: elementList)" then you will only walk the links once. You will need to also keep you own counter so you know what index you are on but that is easy to do with a local "index" that you increment every time around the for loop,. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -135,6 +161,27 @@ public int getDSFID() { return REDIS_LIST_ID; } + public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement, + boolean before) { + for (int i = 0; i < elementList.size(); i++) { + if (Arrays.equals(elementList.get(i), referenceElement)) { + if (before) { + elementList.add(i, elementToInsert); + return i; + } else { + elementList.add(i + 1, elementToInsert); + return i + 1; + } + } + } + + return -1; + } + + public synchronized void elementInsert(byte[] toInsert, int index) { Review comment: I could be wrong but this seems like overkill. Just have the single 'int elementInsert' method. Callers are free to ignore its result. If you do keep this you don't need it to be synchronized since it does not do anything that is not thread safe. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java ########## @@ -0,0 +1,50 @@ +/* + * 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.data.delta; + +import static org.apache.geode.DataSerializer.readByteArray; +import static org.apache.geode.DataSerializer.readInteger; +import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.InternalDataSerializer; +import org.apache.geode.redis.internal.data.AbstractRedisData; + +public class InsertByteArray implements DeltaInfo { + private final byte[] byteArray; + private final int index; + + public InsertByteArray(byte[] delta, int index) { + byteArray = delta; + this.index = index; + } + + public void serializeTo(DataOutput out) throws IOException { + DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out); + InternalDataSerializer.writeInteger(index, out); Review comment: Change this to DataSerializer.writePrimitiveInt ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java ########## @@ -0,0 +1,50 @@ +/* + * 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.data.delta; + +import static org.apache.geode.DataSerializer.readByteArray; +import static org.apache.geode.DataSerializer.readInteger; +import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.InternalDataSerializer; +import org.apache.geode.redis.internal.data.AbstractRedisData; + +public class InsertByteArray implements DeltaInfo { + private final byte[] byteArray; + private final int index; + + public InsertByteArray(byte[] delta, int index) { + byteArray = delta; + this.index = index; + } + + public void serializeTo(DataOutput out) throws IOException { + DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out); + InternalDataSerializer.writeInteger(index, out); + InternalDataSerializer.writeByteArray(byteArray, out); + } + + public static void deserializeFrom(DataInput in, AbstractRedisData redisData) throws IOException { + int index = readInteger(in); Review comment: Change this to DataSerializer.readPrimitiveInt ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -135,6 +161,27 @@ public int getDSFID() { return REDIS_LIST_ID; } + public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement, + boolean before) { + for (int i = 0; i < elementList.size(); i++) { + if (Arrays.equals(elementList.get(i), referenceElement)) { + if (before) { Review comment: Consider something like this: ``` int insertIndex = i; if (!before) { insertIndex++; } elementList.add(insertIndex, elementToInsert); return insertIndex; ``` ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java ########## @@ -0,0 +1,50 @@ +/* + * 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.data.delta; + +import static org.apache.geode.DataSerializer.readByteArray; +import static org.apache.geode.DataSerializer.readInteger; +import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.InternalDataSerializer; +import org.apache.geode.redis.internal.data.AbstractRedisData; + +public class InsertByteArray implements DeltaInfo { + private final byte[] byteArray; + private final int index; + + public InsertByteArray(byte[] delta, int index) { + byteArray = delta; + this.index = index; + } + + public void serializeTo(DataOutput out) throws IOException { + DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out); + InternalDataSerializer.writeInteger(index, out); + InternalDataSerializer.writeByteArray(byteArray, out); Review comment: Change this to DataSerializer. This class should not import InternalDataSerializer. -- 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-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
