Skip to content

Adding handling of FAILING_OVER and FAILED_OVER events/push notifications #3716

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

Draft
wants to merge 1 commit into
base: ps_hitless_upgrade_sync_redis
Choose a base branch
from

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

Adding handling of FAILING_OVER and FAILED_OVER events/push notifications

@petyaslavova petyaslavova requested a review from Copilot July 24, 2025 15:07
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 handling FAILING_OVER and FAILED_OVER events/push notifications in Redis cluster maintenance operations. These new events complete the failover lifecycle alongside the existing migration events.

  • Introduces two new event classes: NodeFailingOverEvent and NodeFailedOverEvent
  • Adds a new maintenance state FAILING_OVER to track failover operations
  • Extends the existing test infrastructure to validate the complete event lifecycle

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
redis/maintenance_events.py Adds new failover event classes and FAILING_OVER state, updates handler to support failover events
tests/test_maintenance_events.py Adds comprehensive test coverage for new failover event classes and updated handler methods
tests/test_maintenance_events_handling.py Extends integration tests to validate failover events in disabled timeout scenarios and state transitions
tests/test_connection_pool.py Removes unused import of MaintenanceState
Comments suppressed due to low confidence (2)

tests/test_maintenance_events_handling.py:805

  • The test validates that FAILING_OVER events are received but doesn't verify that the maintenance_state is properly updated or that the event handler is called. Consider adding assertions to verify the event handling behavior matches the expected state transitions.
            # Command 5: This SET command will receive FAILING_OVER push message before response

tests/test_maintenance_events_handling.py:825

  • Similar to the FAILING_OVER test, this test doesn't verify that the FAILED_OVER event handler is properly called or that maintenance state transitions occur as expected. Consider adding assertions for event handling behavior.
            # Command 7: This SET command will receive FAILED_OVER push message before response

@@ -768,6 +791,57 @@ def test_migrating_event_with_disabled_relax_timeout(self, pool_class):
f"Command 3 (GET key1) failed. Expected: {expected_value3}, Got: {result3}"
)

# Command 4: This SET command will receive MIGRATED push message before response
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment incorrectly states 'MIGRATED push message' when it should say 'MIGRATED push notification' for consistency with other comments in the same test that use 'push message'.

Suggested change
# Command 4: This SET command will receive MIGRATED push message before response
# Command 4: This SET command will receive MIGRATED push notification before response

Copilot uses AI. Check for mistakes.

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.

1 participant