DATAREDIS-803 - Work around Redis parameter limitation #326
Conversation
|
Please create a ticket in our issue tracker so we can understand, what exactly you want to achieve. |
christian-sahlmann
force-pushed
the
master
branch
4 times, most recently
from
b45256e to
d6ceec8
Compare
christian-sahlmann
changed the title
christian-sahlmann
changed the title
christian-sahlmann
changed the title
christian-sahlmann
force-pushed
the
master
branch
from
d6ceec8 to
269dd64
Compare
|
Ticket created: https://jira.spring.io/browse/DATAREDIS-803 |
christian-sahlmann
force-pushed
the
master
branch
5 times, most recently
from
82d1efc to
fee3d47
Compare
There was a problem hiding this comment.
Adding an optimization to fetch all entries with a single command is a good one. We should however not break command atomicity by introducing transparent partitioning if commands grow too big. This change hides Redis limitations and will cause surprises.
Such specifics rather belong into application code.
If you drop the mentioned change from your pull request, we can continue on the map access optimization.
| byte[] rawKey = rawKey(key); | ||
|
|
||
| Map<byte[], byte[]> hashes = new LinkedHashMap<>(m.size()); | ||
| int size = Math.min(RedisTemplate.REDIS_MAX_ARGS / 2 - 1, m.size()); |
There was a problem hiding this comment.
We can't change a single anticipated HMSET to multiple ones as we're losing atomicity. That's rather something to be handled in the application code.
There was a problem hiding this comment.
Good point. I didn't think about the atomicity. Removed it again and adapted the test accordingly.
| */ | ||
| public class RedisTemplate<K, V> extends RedisAccessor implements RedisOperations<K, V>, BeanClassLoaderAware { | ||
|
|
||
| public static final int REDIS_MAX_ARGS = 1024 * 1024; |
There was a problem hiding this comment.
We should remove this one as there's no need for the constant (see comment above about atomicity).
There was a problem hiding this comment.
Yes, removed.
| } | ||
|
|
||
| return entries; | ||
| return hashOps.entries().entrySet(); |
There was a problem hiding this comment.
Nice optimization. We should retain the check for pipelining/transaction via checkResult(…) ending in something like:
entries = hashOps.entries();
checkResult(entries);
return entries.entrySet();
There was a problem hiding this comment.
Thanks. In the first place, I didn't intend this as an optimization, but as a fix to be able to fetch more than 1024*1024 -1 entries. But I agree, the optimization is a nice side effect ;)
I re-added the checkResult(...).
christian-sahlmann
force-pushed
the
master
branch
from
fee3d47 to
d81d990
Compare
Redis has a [limitation of 1024 * 1024 parameters](https://github.com/antirez/redis/blob/4.0.9/src/networking.c#L1200) for bulk operations. To insert more than 1024 * 1024 / 2 - 1 entries with putAll(), they need to be split up in multiple calls. To reveive more than 1024 * 1024 - 1 entries with entrySet(), we can directly use the HGETALL command instead of first fetching the keys with HKEYS and then fetching the values with HMGET.
christian-sahlmann
force-pushed
the
master
branch
from
d81d990 to
d1f8201
Compare
Redis has a limitation of 1024 * 1024 parameters]() for bulk operations. To receive more than 1024 * 1024 - 1 entries with entrySet(), we can directly use the HGETALL command instead of first fetching the keys with HKEYS and then fetching the values with HMGET. See also: https://github.com/antirez/redis/blob/4.0.9/src/networking.c#L1200 Original pull request: #326.
Remove DefaultRedisMapEntry as it's no longer required. Simplify test to unit test to reduce test scope and test run time. Original pull request: #326.
Redis has a limitation of 1024 * 1024 parameters]() for bulk operations. To receive more than 1024 * 1024 - 1 entries with entrySet(), we can directly use the HGETALL command instead of first fetching the keys with HKEYS and then fetching the values with HMGET. See also: https://github.com/antirez/redis/blob/4.0.9/src/networking.c#L1200 Original pull request: #326.
Remove DefaultRedisMapEntry as it's no longer required. Simplify test to unit test to reduce test scope and test run time. Original pull request: #326.
|
Thanks a lot. That's merged, polished, and backported to 2.0.x now. |
Redis has a limitation of 1024 * 1024 parameters for bulk operations.
To insert more than 1024 * 1024 / 2 - 1 entries with putAll(), they need to be split up in multiple HMSET commands.
To reveive more than 1024 * 1024 - 1 entries with entrySet(), we can directly use the HGETALL command instead of first fetching the keys with HKEYS and then fetching the values with HMGET.