-
Notifications
You must be signed in to change notification settings - Fork 1.2k
DATAREDIS-803 - Work around Redis parameter limitation #326
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.
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
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.
b45256e to
d6ceec8
Compare
d6ceec8 to
269dd64
Compare
Ticket created: https://jira.spring.io/browse/DATAREDIS-803
82d1efc to
fee3d47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I didn't think about the atomicity. Removed it again and adapted the test accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(...).
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.