Added HealthcheckRegistry instead of RotationManagement in GRPC Healthcheck #116
Conversation
ajaypj
changed the title
WalkthroughThe changes update multiple components to improve dependency injection and health check handling. The modifications include passing a new HealthCheckRegistry parameter during Environment instantiation, revising health check logic in endpoints and gRPC services, and restructuring the concurrency model in HealthCheckRegistry with a cached thread pool and a dedicated thread factory. Additionally, Guice modules have been modified to pass the bootstrap dependency when instantiating modules. These updates alter constructor signatures and internal logic to streamline dependency management without changing the overall control flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Dashboard as DashboardHealthCheckResource
participant Registry as HealthCheckRegistry
participant Checker as HealthCheck Checkers
Client->>Dashboard: HTTP GET /healthcheck
Dashboard->>Registry: runHealthChecks()
Registry->>Checker: Execute health checks concurrently
Checker-->>Registry: Return individual health check results
Registry-->>Dashboard: Aggregated results
alt Any Unhealthy
Dashboard->>Client: HTTP 503 SERVICE_UNAVAILABLE
else All Healthy
Dashboard->>Client: HTTP 200 OK with results
end
sequenceDiagram
participant GRPCClient as gRPC Client
participant GRPCService as GrpcHealthCheckService
participant Registry as HealthCheckRegistry
participant Checker as HealthCheck Checkers
GRPCClient->>GRPCService: check()
GRPCService->>Registry: runHealthChecks()
Registry->>Checker: Execute health checks concurrently
Checker-->>Registry: Return individual results
Registry-->>GRPCService: Aggregated results
GRPCService-->>GRPCClient: Return SERVING / NOT_SERVING
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
guice/src/main/java/com/flipkart/gjex/guice/module/ServerModule.java (1)
42-48: Add a null check for thebootstrapconstructor parameter.Including a simple null check would prevent potential
NullPointerExceptionissues and improve robustness. For example:public ServerModule(Bootstrap<T,U> bootstrap) { + Preconditions.checkNotNull(bootstrap, "bootstrap must not be null"); this.bootstrap = bootstrap; }core/src/main/java/com/flipkart/gjex/core/healthcheck/GrpcHealthCheckService.java (1)
28-29: Constructor injection.
The constructor injecting theBootstrap<T, U>is aligned with standard dependency injection practices. A minor improvement is to guard against null by adding a null check to provide an early, descriptive failure ifbootstrapis unexpectedly null.public GrpcHealthCheckService(Bootstrap<T, U> bootstrap) { + if (bootstrap == null) { + throw new IllegalArgumentException("bootstrap must not be null"); + } this.bootstrap = bootstrap; }core/src/main/java/com/flipkart/gjex/core/web/HealthCheckResource.java (1)
21-21: Remove unused import.The
javax.inject.Injectimport is not being used in this class.-import javax.inject.Inject;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/com/flipkart/gjex/core/Application.java(2 hunks)core/src/main/java/com/flipkart/gjex/core/healthcheck/GrpcHealthCheckService.java(2 hunks)core/src/main/java/com/flipkart/gjex/core/healthcheck/HealthCheckRegistry.java(2 hunks)core/src/main/java/com/flipkart/gjex/core/setup/Environment.java(1 hunks)core/src/main/java/com/flipkart/gjex/core/web/DashboardHealthCheckResource.java(1 hunks)core/src/main/java/com/flipkart/gjex/core/web/HealthCheckResource.java(2 hunks)guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java(1 hunks)guice/src/main/java/com/flipkart/gjex/guice/module/DashboardModule.java(2 hunks)guice/src/main/java/com/flipkart/gjex/guice/module/ServerModule.java(2 hunks)
🔇 Additional comments (16)
guice/src/main/java/com/flipkart/gjex/guice/module/ServerModule.java (2)
19-19: Imports for new generics approach look good.These imports are necessary for the newly introduced generic parameters and do not raise any immediate red flags.
Also applies to: 22-22, 34-34
56-57: ConfirmGrpcHealthCheckServicethread safety when using.toInstance(...).Because
.toInstance(...)binds a single shared instance, ensure thatGrpcHealthCheckServiceis stateless or otherwise thread-safe to handle concurrent calls.guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java (1)
136-136: Initialization update aligns correctly with the new constructor signature.Passing the
bootstrapparameter is consistent with the changes inServerModule. No issues observed.core/src/main/java/com/flipkart/gjex/core/healthcheck/GrpcHealthCheckService.java (5)
3-4: Imports are consistent and necessary.
These imports align well with the revised health check approach and the added generic parameters.Also applies to: 6-6, 14-15
24-24: Generics for improved flexibility.
Declaring<T extends GJEXConfiguration, U extends Map>promotes type-safety and extensibility. Ensure that callers pass compatible types to avoid class cast issues.
26-26: New bootstrap field for dependency injection.
Storing theBootstrap<T,U>reference as a final field is a clean approach to ensure the class has a consistent, reliable reference to application-level services.
37-38: Concurrent health checks usage.
Callingbootstrap.getHealthCheckRegistry().runHealthChecks()gathers health check results in parallel. This is more dynamic than the old rotation-based check. Be aware of potential partial failures if any single health check is long-running or throws an exception.
40-41: Status setting logic.
UsingNOT_SERVINGif any check fails andSERVINGotherwise is straightforward. No concerns here.core/src/main/java/com/flipkart/gjex/core/healthcheck/HealthCheckRegistry.java (3)
18-19: Imports necessary for concurrency.
These concurrency-focused imports (Executors,ThreadFactory,AtomicInteger) support the new cached thread pool strategy.Also applies to: 22-24
41-47: Overridden runHealthChecks for concurrent execution.
OverridingrunHealthChecks()to use the cached thread pool is coherent with this class’s goal. Confirm that large sets of health checks will not overwhelm the system or cause timeouts.
49-69: Thread naming for maintainability.
Using a customNamedThreadFactoryis a good practice, helping to identify and monitor health check threads. Marking threads as daemons is valid for tasks that shouldn’t block JVM shutdown. Just be mindful of your system’s lifecycle requirements.core/src/main/java/com/flipkart/gjex/core/setup/Environment.java (1)
33-36: Constructor injection of HealthCheckRegistry.
AddingHealthCheckRegistryas a formal dependency improves clarity and testability. Ensure any existing code that constructsEnvironmentis updated accordingly to avoid broken calls.core/src/main/java/com/flipkart/gjex/core/web/HealthCheckResource.java (1)
47-51: LGTM! Good improvement in dependency management.The changes improve the code by:
- Using constructor injection instead of ServletContext
- Making dependencies explicit
- Simplifying health check execution
Also applies to: 56-61
core/src/main/java/com/flipkart/gjex/core/Application.java (1)
108-108: LGTM! Good initialization of HealthCheckRegistry.The Environment instantiation now properly includes HealthCheckRegistry, aligning with the dependency injection improvements.
guice/src/main/java/com/flipkart/gjex/guice/module/DashboardModule.java (2)
224-230: LGTM! Good improvement to dependency injection.The refactoring simplifies the dependency chain by having the method create its own HealthCheckResource instance using the bootstrap's HealthCheckRegistry.
235-242: LGTM! Maintains consistency with getAPIResourceConfig.The refactoring follows the same pattern as getAPIResourceConfig, ensuring a consistent approach to health check resource creation using the bootstrap's HealthCheckRegistry.
| // Creating a cached threadpool as the number of HealthCheck instances are anyway unknown and | ||
| // hence no point in bounding it to a number | ||
| private final ExecutorService executorService = Executors.newCachedThreadPool(new NamedThreadFactory( | ||
| "GJEX-healthcheck-")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cached thread pool initialization.
Creating an unbounded cached thread pool may handle bursts of health checks well but can consume substantial resources if many checks come in simultaneously. Consider validating the potential concurrency load. Also ensure you have a graceful shutdown strategy to avoid resource leaks.
| private HealthCheckRegistry healthCheckRegistry; | ||
|
|
||
| public DashboardHealthCheckResource(HealthCheckRegistry healthCheckRegistry){ | ||
| this.healthCheckRegistry = healthCheckRegistry; | ||
| } |
There was a problem hiding this comment.
Fix inconsistent registry usage.
The class has inconsistent registry usage:
- Registry is injected via constructor
- But
performHealthChecksstill retrieves it from ServletContext
Use the injected registry instead:
public Response performHealthChecks() {
- HealthCheckRegistry registry = (HealthCheckRegistry) servletContext
- .getAttribute(HealthCheckRegistry.HEALTHCHECK_REGISTRY_NAME);
- SortedMap<String, HealthCheck.Result> results = registry.runHealthChecks();
+ SortedMap<String, HealthCheck.Result> results = healthCheckRegistry.runHealthChecks();
if (results.values().stream().anyMatch(result -> !result.isHealthy())){
return Response.status(Response.Status.SERVICE_UNAVAILABLE).entity(results).build();
}
return Response.status(Response.Status.OK).entity(results).build();
}Also applies to: 54-62
2 participants
Summary by CodeRabbit
New Features
Refactor