-
Notifications
You must be signed in to change notification settings - Fork 614
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
ISPN-14671 HSCAN #11051
ISPN-14671 HSCAN #11051
Conversation
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.
I don't see any particular thing but I did not do a deep review neither
server/resp/src/main/java/org/infinispan/server/resp/commands/hash/HSCAN.java
Show resolved
Hide resolved
server/core/src/main/java/org/infinispan/server/iteration/HashMapIterationManager.java
Outdated
Show resolved
Hide resolved
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.
some considerations.
trying to get confidence with the PR
private CompletionStage<RespRequestHandler> iterate(Resp3Handler handler, IterationManager manager, String cursor, | ||
IterationArguments arguments) { | ||
if (manager == null) { | ||
ByteBufferUtils.stringToByteBuf("*2\r\n$1\r\n0\r\n*0\r\n", handler.allocator()); |
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.
better to give a meaningful name to this bytes seq
IterableIterationResult.Status status = result.getStatusCode(); | ||
if (status == IterableIterationResult.Status.InvalidIteration) { | ||
// Let's just return 0 | ||
ByteBufferUtils.stringToByteBuf("*2\r\n$1\r\n0\r\n*0\r\n", handler.allocator()); |
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.
same
@Override | ||
protected CacheStream<CacheEntry<Object, Object>> baseStream(AdvancedCache cache, IterationInitializationContext ctx) { | ||
if (ctx == null) return EMPTY_STREAM; | ||
if (!(ctx instanceof HashMapInitializationContext)) throw new IllegalStateException("Illegal context for iteration"); |
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.
how about turn ctx into a property of the specialized class, remove it from the method params and avoid this check?
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.
Yeah, this was something I was trying at first. I took this approach because the iteration manager is shared between all clients IIUC. So it would need a way to synchronize between retrieving the entry from the cache and invoking start. All this (trying to synchronize or add a ctx) could be avoided if the start method receives the source stream or something similar. Let me think more about this.
|
||
Map<Object, Object> source = ((HashMapInitializationContext) ctx).source; | ||
AbstractLocalCacheStream.StreamSupplier<CacheEntry<Object, Object>, Stream<CacheEntry<Object, Object>>> ss = new HashMapStreamSupplier(source); | ||
return new LocalCacheStream<>(ss, false, SecurityActions.getCacheComponentRegistry(cache)); |
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.
I'm wondering if we really need a full cache stream even to iterate on an inner struct
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.
This one also was a small paint point. I think this one might be necessary as we can use the existing code to apply the filters. But let me think about this, too.
* Create an iterator manager that handles a HashMap source.
https://issues.redhat.com/browse/ISPN-14671
Create an iterator manager that handles a
Map
source. The downside is we have the complete entry loaded in memory to iterate, and the entry could be huge.