Skip to content
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

NodeSelection dispatch output gets reused #1876

Closed
mp911de opened this issue Oct 7, 2021 Discussed in #1875 · 0 comments
Closed

NodeSelection dispatch output gets reused #1876

mp911de opened this issue Oct 7, 2021 Discussed in #1875 · 0 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@mp911de
Copy link
Collaborator

mp911de commented Oct 7, 2021

Discussed in #1875

Originally posted by seeday October 6, 2021
I'm attempting to broadcast commands to all the replicas in a Redis Cluster deployment, but I'm seeing some weird behavior. Code is below. Redis 6.0.9 and Lettuce 6.1.5.

fun doThing(): CompletableFuture<List<Any?>?> {
    return lettuce.connectAsync().thenComposeAsync {
        it.async().replicas().commands().slowlogGet(1)
    }.thenApply {
        it
    }
}

   
fun doThing2(): CompletableFuture<List<Any?>?> {
    return lettuce.connectAsync().thenComposeAsync {
        it.async().replicas().commands().dispatch(
            CommandType.SLOWLOG,
            NestedMultiOutput(lettuce.codec),
            CommandArgs(lettuce.codec).add("GET").add(1)
        )
    }.thenApply {
        it
    }
}

fun doThing3(): CompletableFuture<List<Any?>?> {
    return lettuce.connectAsync().thenComposeAsync { conn ->
        val futures = conn.async().replicas().asMap().values.map {
            it.dispatch(
                CommandType.SLOWLOG,
                NestedMultiOutput(lettuce.codec),
                CommandArgs(lettuce.codec).add("GET").add(1)
            )
        }
        CompletableFuture.allOf(*futures.toArray { arrayOfNulls<CompletableFuture<*>>(it) })
            .thenApply { futures }
    }.thenApply {
        it.map { it.get() }
    }
}

doThing and doThing3 work just fine, however I feel like doThing2 should also work, and I don't quite understand why. The results from doThing2 return jumbled up, weirdly nested, and out of order, like there's multiple threads fighting over the single NestedMultiOutput. I've tried several ways of synchronizing a NestedMultiOutput, but neither locks nor threadlocals work properly.

While 3 does work, (specifically for custom commands), I'm curious as to what option 1 is doing to synchronize the responses properly, why option 2 is not equivalent to 1, and if it's doing it faster than my mess of CompletableFutures in option 3.

Thanks!

We need to document that dispatch cannot be used in its regular form as the CommandOutput is shared across connections and that messes up the result. We should also throw an exception there and consider a dispatch variant accepting a Supplier.

@mp911de mp911de added the type: task A general task label Oct 7, 2021
@mp911de mp911de changed the title NodeSelection dispatch output gets reused. NodeSelection dispatch output gets reused Oct 7, 2021
@mp911de mp911de added this to the 6.2.0 milestone Feb 28, 2022
@mp911de mp911de added type: enhancement A general enhancement and removed type: task A general task labels Feb 28, 2022
mp911de added a commit that referenced this issue Feb 28, 2022
…tput #1876

The command output from dispatch(…) on the NodeSelection API is being reused for all command instances and therefore it collects responses from all nodes. This is typically not desired. Therefore, we introduced dispatch(…) method overloads accepting Supplier of outputs to provide an individual output for each invocation to separate responses. dispatch(…) methods accepting just an output object are deprecated now.
@mp911de mp911de closed this as completed Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

1 participant