-
-
Notifications
You must be signed in to change notification settings - Fork 992
When being moved to a different connection, reuse the parameters (apa… #1266
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
base: main
Are you sure you want to change the base?
Conversation
* Add missing `@return` annotations * formatting * formatting * Update ProcessorChain.php * Update Handler.php --------- Co-authored-by: Till Krüss <tillkruss@users.noreply.github.com>
…rt from host & port) from a random previously established connection. Closes 1259
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 needs some unit tests.
@tillkruss @MrKickkiller This PR looks like a workaround. You shouldn't be able to communicate with "undefined" or "hidden" nodes, if you need to then it's a problem of Redis infrastructure. I would recommend to close this PR and start investigation on MemoryDB Redis infrastructure. I left a comment within #1259 that describes it better. You should never have MOVED or ASK response from Redis server if you communicate via proxy, because it's a proxy responsibility to redirect it to the correct node, not a library. |
@MrKickkiller UPD: This PR could make sense if other client also have a "hack" for MemoryDB connection, let me ensure. Also, it's a breaking change, isn't it? @tillkruss |
…as it isn't needed or used in predis#1266
@tillkruss @MrKickkiller I'm okay to approve, but it's looks like a breaking change? |
It is, so I'd would need to go into v3.0 which does not have a release date. Maybe towards the end of the year. |
Tests were failing since the Parameters.create method did not function correctly and would quickly fall back to defaults.
* | ||
* @return NodeConnectionInterface | ||
*/ | ||
protected function createConnection($connectionID) | ||
protected function createConnection(string $connectionID, ParametersInterface $src = null): NodeConnectionInterface |
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 8000 more.
Please rebase your PR to main
. We cannot accept breaking changes in v2.x
.
…rt from host & port) from a random previously established connection.
Closes #1259
Edits are welcome, as I am very sure that I may have broken some PHP conventions, project conventions etc.
The goal of this PR was to allow the library to work with TLS Redis cluster, where by only a subset of nodes is given in configuration, but the client may be asked/moved to different, undefined nodes.