[
https://issues.apache.org/jira/browse/GEODE-8130?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17107639#comment-17107639
]
ASF GitHub Bot commented on GEODE-8130:
---------------------------------------
sabbeyPivotal commented on a change in pull request #5067:
URL: https://github.com/apache/geode/pull/5067#discussion_r425384495
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -526,20 +499,28 @@ private void registerLockServiceMBean() {
}
private void checkForRegions() {
- Collection<Entry<String, RedisDataType>> entrySet =
keyRegistrar.keyInfos();
- for (Entry<String, RedisDataType> entry : entrySet) {
- String regionName = entry.getKey();
- RedisDataType type = entry.getValue();
- Region<?, ?> newRegion = cache.getRegion(regionName);
- if (newRegion == null && type != RedisDataType.REDIS_STRING && type !=
RedisDataType.REDIS_HLL
- && type != RedisDataType.REDIS_PROTECTED) {
- try {
- regionCache
-
.createRemoteRegionReferenceLocally(Coder.stringToByteArrayWrapper(regionName),
type);
- } catch (Exception e) {
- if (logger.errorEnabled()) {
- logger.error(e);
- }
+ Collection<Entry<ByteArrayWrapper, RedisData>> entrySet =
keyRegistrar.keyInfos();
+ for (Entry<ByteArrayWrapper, RedisData> entry : entrySet) {
+ ByteArrayWrapper key = entry.getKey();
+ RedisDataType type = entry.getValue().getType();
+ if (!regionProvider.typeUsesDynamicRegions(type)) {
+ continue;
+ }
+ if (cache.getRegion(key.toString()) != null) {
+ // TODO: this seems to be correct (i.e. no need to call
createRemoteRegionReferenceLocally
+ // if region already exists).
+ // HOWEVER: createRemoteRegionReferenceLocally ends up doing nothing
if the region does not
+ // exist. So this caller of createRemoteRegionReferenceLocally
basically does nothing.
+ // createRemoteRegionReferenceLocally might be needed even if the
region exists because
+ // local state needs to be initialized (like indexes and queries).
+ continue;
+ }
+ try {
+ regionProvider.createRemoteRegionReferenceLocally(key, type);
+ } catch (Exception e) {
+ // TODO: this eats the exception so if something really is wrong we
don't fail but just log.
+ if (logger.errorEnabled()) {
+ logger.error(e);
}
Review comment:
I wonder if this is necessary for some local state... I guess we may
find out more once we implement Sorted Sets and Lists? Not sure how dangerous
removing it is.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RenameExecutor.java
##########
@@ -64,6 +69,24 @@ public void executeCommand(Command command,
ExecutionHandlerContext context) {
region.put(newKey, value);
removeEntry(key, redisDataType, context);
break;
+ case REDIS_HASH:
+ // TODO this all needs to be done atomically. Add RENAME support
to RedisHashCommands
+ RedisHashCommands redisHashCommands =
+ new
RedisHashCommandsFunctionExecutor(context.getRegionProvider().getDataRegion());
+ Collection<ByteArrayWrapper> fieldsAndValues =
redisHashCommands.hgetall(key);
+ redisHashCommands.del(key);
+ redisHashCommands.del(newKey);
+ redisHashCommands.hset(newKey, new ArrayList<>(fieldsAndValues),
false);
+ break;
+ case REDIS_SET:
+ // TODO this all needs to be done atomically. Add RENAME support
to RedisSetCommands
Review comment:
should we put a placeholder story in the icebox for adding RENAME
support for RedisHashCommands and RedisSetCommands?
----------------------------------------------------------------
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:
[email protected]
> Change the way redis uses geode regions to save memory and fix race conditions
> ------------------------------------------------------------------------------
>
> Key: GEODE-8130
> URL: https://issues.apache.org/jira/browse/GEODE-8130
> Project: Geode
> Issue Type: Improvement
> Components: redis
> Reporter: Darrel Schneider
> Assignee: Darrel Schneider
> Priority: Major
>
> Currently the geode redis implementation uses a replicate region to hold
> every redis key as the region key (as a String object) and the region value
> is an enum describing the type. Another partitioned regioni is used to also
> store the redis key as the region key (as a ByteArrayWrapper object) and the
> region value is an object that is basically a Set or HashMap.
> This uses extra memory, the replicate does not scale well, is slower
> (updating two regions is slower than one region), and trying to update two
> regions to hold the same logical redis "key" has race conditions which can
> lead to inconsistent data.
> The solution is to have a single partitioned region that can hold multiple
> types of data (sets, hashes, strings, lists, etc).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
