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

[controller] Checking parent controller region state before handling requests for all routes #1075

Merged
merged 7 commits into from
Jul 29, 2024

Conversation

bonytoni
Copy link
Collaborator

@bonytoni bonytoni commented Jul 19, 2024

Checking parent controller region state before handling requests for all routes

  • Implemented two region states in ParentControllerRegionState
    • ACTIVE - parent controller serves requests
    • PASSIVE - parent controller rejects requests
  • VeniceParentControllerRegionStateHandler is a wrapper class for Route that checks the state of the region of the parent controller before handling requests. This class was used in AdminSparkServer to wrap routes in:
    • AdminCommandExecutionRoutes
    • AdminTopicMetadataRoutes
    • ClusterDiscovery
    • ClusterRoutes
    • ControllerRoutes
    • CreateStore
    • CreateVersion
    • DataRecoveryRoutes
    • JobRoutes
    • MigrationRoutes
    • NewClusterBuildOutRoutes
    • NodesAndReplicas
    • RoutersClusterConfigRoutes
    • SchemaRoutes
    • SkipAdminRoute
    • StoragePersonaRoutes
    • StoresRoutes
    • VersionRoute
  • Added config key CONTROLLER_PARENT_REGION_STATE in ConfigKeys to retrieve the config value from the backend

Next steps: Implement the parent controller region state config in the backend

How was this PR tested?

Unit Tests, Integration Tests

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

@bonytoni bonytoni marked this pull request as ready for review July 22, 2024 23:19
@bonytoni bonytoni changed the title [WIP][controller] Checking controller state before handling requests [controller] Checking parent controller state before handling requests Jul 22, 2024
@bonytoni bonytoni force-pushed the RegionStatesHandleRequests branch from bac4d5e to 5c3f6a3 Compare July 26, 2024 01:50
@bonytoni bonytoni changed the title [controller] Checking parent controller state before handling requests [controller] Checking parent controller region state before handling requests for all routes Jul 26, 2024
nisargthakkar
nisargthakkar previously approved these changes Jul 29, 2024
Copy link
Contributor

@nisargthakkar nisargthakkar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Tony!

Left one query. If you think it doesn't apply, feel free to resolve it and merge the PR

bonytoni added 6 commits July 29, 2024 14:54
…h VeniceParentControllerRegionStateHandler"

This reverts commit 1c03f5b.
- Added ACTIVE checks to all routes in AdminSparkServer using VeniceParentControllerRegionStateHandler wrapper class
- Reverted CreateVersionTest to previous commit without ParentControllerRegionState.ACTIVE
- Removed some minor parentheses in ParentControllerRegionState
@bonytoni bonytoni force-pushed the RegionStatesHandleRequests branch from 8e492e1 to df3f9b9 Compare July 29, 2024 21:55
Copy link
Contributor

@nisargthakkar nisargthakkar left a comment

Choose a reason for hiding this comment

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

Looking quite good. Just the two test-related changes needed

Copy link
Contributor

@nisargthakkar nisargthakkar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @bonytoni !

@bonytoni bonytoni merged commit cdbd176 into linkedin:main Jul 29, 2024
32 checks passed
@bonytoni bonytoni deleted the RegionStatesHandleRequests branch July 29, 2024 23:57
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.

2 participants