Skip to content

Conversation

petyaslavova
Copy link
Collaborator

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please provide a description of the change here.

@petyaslavova petyaslavova requested a review from Copilot August 29, 2025 06:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for maintenance push notifications handling during server upgrade or maintenance procedures. It introduces a comprehensive maintenance events system that allows Redis clients to gracefully handle node movements, migrations, and failovers in Redis clusters by adjusting connection timeouts and implementing proactive reconnection strategies.

Key changes include:

  • Addition of new maintenance event classes and configuration system
  • Integration of maintenance event handlers into connection pools and clients
  • Support for RESP3 push notifications for maintenance events
  • Enhanced connection pool management with maintenance-aware operations

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
redis/maintenance_events.py Core maintenance events system with event classes, configuration, and handlers
redis/connection.py Enhanced connection interface and pool management with maintenance event support
redis/client.py Client-level integration of maintenance events configuration
redis/_parsers/base.py Push notification parsing for maintenance events
redis/_parsers/resp3.py RESP3 parser updates for maintenance notifications
redis/_parsers/hiredis.py Hiredis parser updates for push notification handling
tests/test_maintenance_events.py Comprehensive test suite for maintenance events functionality
tests/test_connection_pool.py Minor test updates for connection pool changes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

socket_connect_timeout: Optional[float] = None,
retry_on_timeout: bool = False,
retry_on_error=SENTINEL,
retry_on_error: Union[Iterable[Type[Exception]], object] = SENTINEL,
Copy link

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

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

[nitpick] The type annotation allows both Iterable[Type[Exception]] and object, which is too permissive. Consider using a more specific union type or creating a dedicated type alias to make the API clearer for consumers.

Suggested change
retry_on_error: Union[Iterable[Type[Exception]], object] = SENTINEL,
retry_on_error: Union[Iterable[Type[Exception]], type(SENTINEL)] = SENTINEL,

Copilot uses AI. Check for mistakes.

_FAILED_OVER_MESSAGE,
)

MSG_TYPE_TO_EVENT_PARSER_MAPPING: dict[str, tuple[type[MaintenanceEvent], Callable]] = {
Copy link

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

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

The type annotation uses Callable without specifying the signature, making it unclear what parameters the parser functions should accept. Consider using Callable[[List[Any]], MaintenanceEvent] or a Protocol for better type safety.

Copilot uses AI. Check for mistakes.

@petyaslavova petyaslavova merged commit 4cdf082 into master Aug 29, 2025
92 of 94 checks passed
@petyaslavova petyaslavova deleted the feat/hitless-upgrade-sync-standalone branch August 29, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants