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

Truncate pipeline exception message to a sane size #3530

Merged
merged 14 commits into from
Mar 25, 2025

Conversation

rohansingh
Copy link
Contributor

@rohansingh rohansingh commented Feb 25, 2025

Fixes #3491.

@rohansingh rohansingh marked this pull request as draft February 25, 2025 23:28
@rohansingh rohansingh marked this pull request as ready for review February 25, 2025 23:57
@petyaslavova
Copy link
Collaborator

Hi @rohansingh thank you for your contribution! Your change will be reviewed soon.

@rohansingh rohansingh force-pushed the truncate-pipeline-exception branch from 17a16c6 to 2778765 Compare February 26, 2025 16:37
@rohansingh
Copy link
Contributor Author

  • Fixed lint errors.
  • Fixed truncation length logic.
  • Added truncation in a couple places that it was missed. It's now in all of the following places, as well as their corresponding test files:
    • client.py
    • cluster.py
    • asyncio/client.py
    • asyncio/cluster.py

@vladvildanov
Copy link
Collaborator

@rohansingh Hi! Looks like some cluster tests are still failing

@rohansingh
Copy link
Contributor Author

Sorry, I've had trouble getting the cluster tests to run in my dev environment for some reason. Will take a look in a bit.

Copy link
Collaborator

@petyaslavova petyaslavova left a comment

Choose a reason for hiding this comment

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

The requested changes are in previously posted comments. This update is just to have the correct status in the PRs list.

@vladvildanov
Copy link
Collaborator

LGTM, just need to resolve conflicts and fix tests

@rohansingh
Copy link
Contributor Author

Yup sorry about the delay, I will get to this today! I appreciate the prompt reviews.

@rohansingh rohansingh force-pushed the truncate-pipeline-exception branch from 4599a56 to 0bd8365 Compare March 10, 2025 16:01
Copy link
Collaborator

@petyaslavova petyaslavova left a comment

Choose a reason for hiding this comment

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

Hi @rohansingh , linters are failing and I have one more small request to rename the new function. It can truncate any text and we can use it for other purposes as well.

For the linters, have in mind that we have updated the tooling recently and you might need install again the dev dependencies.

petyaslavova and others added 11 commits March 13, 2025 12:13
* skip `ssl` import if not available

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

* address review feedback and fix lint errors

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

* remove TYPE_CHECKING clause from ssl conditional

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

---------

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Co-authored-by: petyaslavova <petya.slavova@redis.com>
…ameter. Updating pipeline infra (redis#3564)

* Fixing search module dropindex function not to send invalid third parameter. Updating pipeline infra

* Fixing linters
* fix: add TimeoutError handling in get_connection()

In get_connection() we can implicitly call read on
a connection. This can timeout of the underlying TCP
session is gone. With this change we remove it
from the connection pool and get a new connection.

* fix: add TimeoutError handling in get_connection() sync/async

In get_connection() we can implicitly call read on a connection. This
can timeout of the underlying TCP session is gone. With this change we
remove it from the connection pool and get a new connection.

* fix: update version number to match test expectations

* fix: revert version number to 5.2.1, manually uninstall entraid
…cement for 'read_from_replicas' config) (redis#3563)

* Adding laod balancing strategy configuration to cluster clients(replacement for 'read_from_replicas' config)

* Fixing linter errors

* Changing the LoadBalancingStrategy type hints to be defined as optional. Fixed wording in pydocs

* Adding integration tests with the different load balancing strategies for read operation

* Fixing linters
@petyaslavova petyaslavova merged commit 55a50a2 into redis:master Mar 25, 2025
41 checks passed
@petyaslavova petyaslavova added the maintenance Maintenance (CI, Releases, etc) label Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResponseError from pipelines can be extremely large
8 participants