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

When being moved to a different connection, reuse the parameters (apa… #1266

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MrKickkiller
Copy link

@MrKickkiller MrKickkiller commented Apr 26, 2023

…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.

tillkruss and others added 4 commits April 13, 2023 09:51
* 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
Copy link
Member

@tillkruss tillkruss left a 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.

@vladvildanov
Copy link
Contributor

@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.

@vladvildanov
Copy link
Contributor

@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

@coveralls
Copy link

coveralls commented Apr 27, 2023

Coverage Status

Coverage: 83.926% (+0.03%) from 83.894% when pulling f2f65fb on MrKickkiller:fix/predis-1259 into e8ea05d on predis:v2.x.

@vladvildanov
Copy link
Contributor

@tillkruss @MrKickkiller I'm okay to approve, but it's looks like a breaking change?

@tillkruss
Copy link
Member

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.

Mathieu Coussens added 2 commits May 3, 2023 08:12
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase your PR to main. We cannot accept breaking changes in v2.x.

@MrKickkiller MrKickkiller changed the base branch from v2.x to main May 4, 2023 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Connection scheme (rediss) seems to be ignored when retrieving (new) clients from the IteratorAggregate
5 participants