feat(health,version): add health and version endpoints#115
Merged
drtechie merged 14 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#115drtechie merged 14 commits intoPSMRI:mainfrom
drtechie merged 14 commits intoPSMRI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds build-time git metadata generation, a concurrent HealthService performing MySQL (+ optional Redis) checks, a /health REST controller mapping status to HTTP 200/503, refactors /version to read git.properties and return JSON, and excludes /health and /version from the JWT filter. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HealthController
participant HealthService
participant ThreadPool
participant MySQL
participant Redis
Client->>HealthController: GET /health
HealthController->>HealthService: checkHealth()
HealthService->>ThreadPool: submit MySQL check
HealthService->>ThreadPool: submit Redis check (if configured)
par Parallel
ThreadPool->>MySQL: SELECT 1 (with timeout)
ThreadPool->>Redis: PING (with timeout)
end
MySQL-->>ThreadPool: result / error / timeout
Redis-->>ThreadPool: result / error / timeout
ThreadPool-->>HealthService: per-component results
HealthService->>HealthService: aggregate statuses → UP/DEGRADED/DOWN
HealthService-->>HealthController: health map (components, statuses, timestamp)
alt Overall UP
HealthController-->>Client: HTTP 200 + health JSON
else
HealthController-->>Client: HTTP 503 + health JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/iemr/inventory/service/health/HealthService.java (2)
96-119: Health checks run sequentially; consider parallelizing.
checkHealth()invokes MySQL and Redis checks one after the other. In the worst case (both time out), this endpoint blocks for up to 6 seconds (3s + 3s). Since you already have a 2-thread pool andCompletableFutureinside each check, you could launch both checks concurrently andjointhem, halving the worst-case latency. This matters for load-balancer health probes that typically expect sub-second responses.Sketch of parallel execution
public Map<String, Object> checkHealth() { Map<String, Object> response = new LinkedHashMap<>(); response.put("timestamp", Instant.now().toString()); - Map<String, Map<String, Object>> components = new LinkedHashMap<>(); - - // Check MySQL - Map<String, Object> mysqlStatus = new LinkedHashMap<>(); - performHealthCheck("MySQL", mysqlStatus, this::checkMySQLHealth); - components.put("mysql", mysqlStatus); - - // Check Redis - Map<String, Object> redisStatus = new LinkedHashMap<>(); - performHealthCheck("Redis", redisStatus, this::checkRedisHealth); - components.put("redis", redisStatus); + Map<String, Object> mysqlStatus = new LinkedHashMap<>(); + Map<String, Object> redisStatus = new LinkedHashMap<>(); + + CompletableFuture<Void> mysqlFuture = CompletableFuture.runAsync( + () -> performHealthCheck("MySQL", mysqlStatus, this::checkMySQLHealth), executorService); + CompletableFuture<Void> redisFuture = CompletableFuture.runAsync( + () -> performHealthCheck("Redis", redisStatus, this::checkRedisHealth), executorService); + + CompletableFuture.allOf(mysqlFuture, redisFuture).join(); + + Map<String, Map<String, Object>> components = new LinkedHashMap<>(); + components.put("mysql", mysqlStatus); + components.put("redis", redisStatus); response.put("components", components);Note: if you do this, the inner
checkMySQLHealth/checkRedisHealthmethods should no longer submit toexecutorService(to avoid thread-pool starvation with a pool of 2). Run them directly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 96 - 119, checkHealth currently runs performHealthCheck for MySQL and Redis sequentially which doubles worst-case latency; modify checkHealth to launch both component checks concurrently using CompletableFuture.supplyAsync (or similar) for performHealthCheck invocations for "MySQL" and "Redis", then join both futures and collect their results into the components map before computing overall status with computeOverallStatus; also update checkMySQLHealth and checkRedisHealth to execute synchronously (remove their internal executorService submission) so they don't submit additional tasks and risk thread-pool starvation.
158-161: Redis "not configured" uses theerrorfield for a non-error message.When
redisTemplateis null, the result isisHealthy=truewith a message in theerrorfield (line 218–220 populates"error"fromresult.error). This produces a confusing response wherestatus=UPbut anerrorkey is present. Consider using a separate"message"or"info"field for informational notes to avoid confusion with actual errors.Proposed approach
Add a
messagefield toHealthCheckResultfor informational messages, keepingerrorfor actual failures:private static class HealthCheckResult { final boolean isHealthy; final String error; + final String message; - HealthCheckResult(boolean isHealthy, String error) { + HealthCheckResult(boolean isHealthy, String error, String message) { this.isHealthy = isHealthy; this.error = error; + this.message = message; } }Then in
checkRedisHealth:if (redisTemplate == null) { - return new HealthCheckResult(true, "Redis not configured — skipped"); + return new HealthCheckResult(true, null, "Redis not configured — skipped"); }And in
performHealthCheck, populate"message"whenresult.message != null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 158 - 161, Health check currently writes the "Redis not configured" text into HealthCheckResult.error, which is confusing; add a separate informational field (e.g., String message) to the HealthCheckResult class and use it for non-error notes, update checkRedisHealth() to return new HealthCheckResult(true, null, "Redis not configured — skipped") (or equivalent constructor/setter) when redisTemplate is null, ensure actual failures still populate error, and modify performHealthCheck() to populate the response "message" (from result.getMessage()) instead of placing that text into the "error" field so error remains reserved for real failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/inventory/controller/health/HealthController.java`:
- Around line 65-70: The current HealthController uses overallStatus (from
HealthService.computeOverallStatus()) to map any non-"UP" value to
HttpStatus.SERVICE_UNAVAILABLE; change the mapping so that both "UP" and
"DEGRADED" map to HttpStatus.OK and only "DOWN" maps to
HttpStatus.SERVICE_UNAVAILABLE. Locate the code that reads overallStatus and
constructs the ResponseEntity (the block using variables overallStatus,
httpStatus and returning new ResponseEntity<>(healthStatus, httpStatus)) and
update the conditional to explicitly check for "UP" or "DEGRADED" before
assigning HttpStatus.OK, leaving SERVICE_UNAVAILABLE only for "DOWN" (or other
explicit failure states).
---
Nitpick comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 96-119: checkHealth currently runs performHealthCheck for MySQL
and Redis sequentially which doubles worst-case latency; modify checkHealth to
launch both component checks concurrently using CompletableFuture.supplyAsync
(or similar) for performHealthCheck invocations for "MySQL" and "Redis", then
join both futures and collect their results into the components map before
computing overall status with computeOverallStatus; also update checkMySQLHealth
and checkRedisHealth to execute synchronously (remove their internal
executorService submission) so they don't submit additional tasks and risk
thread-pool starvation.
- Around line 158-161: Health check currently writes the "Redis not configured"
text into HealthCheckResult.error, which is confusing; add a separate
informational field (e.g., String message) to the HealthCheckResult class and
use it for non-error notes, update checkRedisHealth() to return new
HealthCheckResult(true, null, "Redis not configured — skipped") (or equivalent
constructor/setter) when redisTemplate is null, ensure actual failures still
populate error, and modify performHealthCheck() to populate the response
"message" (from result.getMessage()) instead of placing that text into the
"error" field so error remains reserved for real failures.
src/main/java/com/iemr/inventory/controller/health/HealthController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/com/iemr/inventory/service/health/HealthService.java (1)
199-201:performHealthCheckhas an unused return value — consider making itvoid.The method signature declares
Map<String, Object>as the return type and returns thestatusparameter, but both callers (Lines 105, 110) ignore it entirely, relying on in-place mutation of thestatusmap. The dual contract (mutate-in-place + return-same-reference) is confusing. Changing the signature tovoidremoves the ambiguity.♻️ Proposed refactor
- private Map<String, Object> performHealthCheck(String componentName, - Map<String, Object> status, - Supplier<HealthCheckResult> checker) { + private void performHealthCheck(String componentName, + Map<String, Object> status, + Supplier<HealthCheckResult> checker) { ... - return status; ... - return status; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 199 - 201, The performHealthCheck method currently returns the status Map but callers ignore that return; change the signature of performHealthCheck(String componentName, Map<String, Object> status, Supplier<HealthCheckResult> checker) to return void and keep the existing in-place mutation of the status map (using HealthCheckResult from the checker). Update all call sites that currently call performHealthCheck(...) (they currently ignore the return) to call it as a void method (remove any assignment/expectation of a returned Map). Ensure imports/types (Supplier, HealthCheckResult) remain unchanged and tests/usage still rely on the mutated status map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 125-127: The JDBC query timeout and the outer future.get() use the
same MYSQL_TIMEOUT_SECONDS causing a race; in HealthService (the method that
creates the PreparedStatement and later calls future.get(...)), change the outer
future.get timeout to use MYSQL_TIMEOUT_SECONDS + 1 (or otherwise add a 1-second
buffer) so the statement's stmt.setQueryTimeout(MYSQL_TIMEOUT_SECONDS) can
trigger first and clean up before the future.get times out.
- Around line 159-162: The informational Redis skip response is being stored
under the "error" key; update the health model and mapping so informational text
is stored under "message" not "error". Add a message field to HealthCheckResult
(alongside existing ok/error), adjust checkRedisHealth() to return the message
when redisTemplate==null (e.g., new HealthCheckResult(true, null, "Redis not
configured — skipped")), and modify performHealthCheck() to put result.message
into status.put("message", ...) when result.ok is true and result.error remains
null, while keeping error handling unchanged for failing checks.
- Around line 189-192: In checkRedisHealth, the InterruptedException handler
logs before restoring the interrupt flag; change the handler to first call
Thread.currentThread().interrupt() to restore the interrupt status and then
perform logger.warn(...) and return the HealthCheckResult(false, "Redis health
check was interrupted") so the interrupt flag isn't lost (mirror the order used
in checkMySQLHealth).
- Around line 97-119: checkHealth currently invokes performHealthCheck
sequentially leading to up-to-6s blocking and false timeouts; make the component
checks run concurrently and ensure the overall timeout is applied across both.
Change checkMySQLHealth and checkRedisHealth to be synchronous helpers (remove
internal CompletableFuture/submit-to-executorService), then in checkHealth
submit both performHealthCheck tasks to executorService as Callables (reference:
performHealthCheck, checkHealth, checkMySQLHealth, checkRedisHealth,
executorService) and wait for both results using invokeAll or by submitting two
Futures and waiting with a single shared timeout (e.g., 3s) so you join both
within the same deadline; keep computeOverallStatus usage the same and populate
mysql/redis maps from the concurrent results, marking timed-out or failed tasks
as DOWN.
- Around line 143-148: The health check uses CompletableFuture and calls
future.cancel(true) in HealthService (MySQL check around the get with
MYSQL_TIMEOUT_SECONDS and similarly the Redis check) but
CompletableFuture.cancel(true) does not interrupt the underlying JDBC/Redis
task; change the implementation to submit the blocking DB/Redis call via
ExecutorService.submit(Callable) so you get a cancellable Future (FutureTask)
and keep that Future reference to call cancel(true) to interrupt the thread, or
alternatively ensure the JDBC/Redis command-level timeout is shorter than
MYSQL_TIMEOUT_SECONDS so the task self-terminates before cancellation; update
both the MySQL health-check path and the Redis health-check path accordingly and
remove reliance on CompletableFuture.cancel(true).
---
Nitpick comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 199-201: The performHealthCheck method currently returns the
status Map but callers ignore that return; change the signature of
performHealthCheck(String componentName, Map<String, Object> status,
Supplier<HealthCheckResult> checker) to return void and keep the existing
in-place mutation of the status map (using HealthCheckResult from the checker).
Update all call sites that currently call performHealthCheck(...) (they
currently ignore the return) to call it as a void method (remove any
assignment/expectation of a returned Map). Ensure imports/types (Supplier,
HealthCheckResult) remain unchanged and tests/usage still rely on the mutated
status map.
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/iemr/inventory/controller/health/HealthController.java (2)
60-70: Health check logging at INFO level will flood logs in production.Load balancers and orchestrators typically poll
/healthevery few seconds. Logging atINFOon Line 61 will generate significant noise. Demote toDEBUG; keep the completion log on Line 69 atDEBUGas well.Proposed fix
- logger.info("Health check endpoint called"); + logger.debug("Health check endpoint called");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/controller/health/HealthController.java` around lines 60 - 70, In HealthController.checkHealth(), change the initial logger call from logger.info("Health check endpoint called") to logger.debug(...) to avoid INFO-level noise from frequent health probes; keep the existing completion log (logger.debug("Health check completed with status: {}"...)) as-is so both entry and exit are at DEBUG level and the method behavior and returned ResponseEntity remain unchanged.
72-81: Error response omits component details, making it hard to distinguish from a healthy-but-DOWN response.The catch-all error response contains only
"status"and"timestamp", while the happy-path response includes"components". Consumers parsing/healthmay expect a stable schema. Consider adding a minimal"error"field with a sanitized message so the caller can distinguish an internal failure from a component-level DOWN.Proposed fix
Map<String, Object> errorResponse = Map.of( "status", "DOWN", - "timestamp", Instant.now().toString() + "timestamp", Instant.now().toString(), + "error", "Unexpected internal error during health check" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/controller/health/HealthController.java` around lines 72 - 81, The catch block in HealthController returns a minimal errorResponse missing "components" and an "error" detail; update the ResponseEntity returned in the catch of the health-check method to include a components object (e.g., an empty or minimal Map under "components") and an "error" field with a sanitized message (not stack trace) alongside "status" and "timestamp" so the response schema matches the happy path; adjust the Map construction used for errorResponse and keep the logger.error("Unexpected error during health check", e) as-is while returning ResponseEntity<>(errorResponse, HttpStatus.SERVICE_UNAVAILABLE).src/main/java/com/iemr/inventory/service/health/HealthService.java (3)
73-77: Thread pool creates non-daemon threads — consider a named thread factory.
Executors.newFixedThreadPool(2)creates non-daemon threads with default names likepool-N-thread-M. If@PreDestroydoesn't fire (e.g., abnormal shutdown), these threads will prevent JVM exit. A customThreadFactorywith daemon threads and a descriptive name (e.g.,health-check-) aids debugging and ensures clean shutdown.Proposed fix
- this.executorService = Executors.newFixedThreadPool(2); + this.executorService = Executors.newFixedThreadPool(2, r -> { + Thread t = new Thread(r, "health-check-" + System.nanoTime()); + t.setDaemon(true); + return t; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 73 - 77, The constructor in HealthService currently uses Executors.newFixedThreadPool(2) which creates non‑daemon, generically named threads; replace that with a fixed thread pool constructed with a custom ThreadFactory that sets threads as daemon and gives them a descriptive name prefix (e.g., "health-check-") so they don't block JVM exit if `@PreDestroy` isn't invoked and are easier to identify in logs; update the HealthService constructor to build that ThreadFactory and pass it to Executors.newFixedThreadPool, keeping executorService as the same field.
239-266:performHealthCheckreturns the same map reference it mutates — return type is misleading.The method accepts
Map<String, Object> status, mutates it in place, and returns it. Callers at Lines 106/108 discard the return value (used withrunAsync+Runnable). Either change the return type tovoid(cleaner contract) or return a new map (functional style). Mutating a shared reference with an unused return is a footgun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 239 - 266, performHealthCheck currently mutates the passed-in Map and returns it, which is misleading because callers (used with runAsync/Runnable) ignore the return; change the method signature from Map<String,Object> performHealthCheck(...) to void performHealthCheck(...), remove the return statement, and keep the existing in-place updates (STATUS_KEY, "responseTimeMs", SEVERITY_KEY, error/message handling, and determineSeverity/HealthCheckResult usage) so callers that pass the status map continue to see the updates without a returned value.
189-237: Remove dead deprecatedprivatemethods.
checkMySQLHealth()andcheckRedisHealth()areprivateand@Deprecated(forRemoval = true). Private methods cannot have external callers, so@Deprecatedannotations serve no purpose — they only add confusion. SincecheckHealth()now calls the sync variants directly, these methods are dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 189 - 237, Remove the dead, private deprecated wrappers checkMySQLHealth() and checkRedisHealth(): they wrap async CompletableFuture calls to checkMySQLHealthSync and checkRedisHealthSync but are unused since checkHealth() now calls the sync variants directly; delete both methods (including their CompletableFuture usage and related timeout/exception handling) and ensure no remaining references to checkMySQLHealth or checkRedisHealth exist (leave checkMySQLHealthSync, checkRedisHealthSync, MYSQL_TIMEOUT_SECONDS, REDIS_TIMEOUT_SECONDS, and executorService intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 97-139: checkHealth currently races and treats timed-out
components as UP because cancelled futures may leave mysqlStatus/redisStatus
empty; after the CompletableFuture.allOf(...) join/cancel block, add logic to
detect unpopulated component maps and mark them DOWN (use
STATUS_KEY/SEVERITY_KEY with STATUS_DOWN/SEVERITY_CRITICAL and an "error"
message referencing the component name) — implement this as a helper like
ensurePopulated(Map<String,Object> status, String componentName) and call it for
mysqlStatus and redisStatus before building components and calling
computeOverallStatus; also avoid the data race by switching
mysqlStatus/redisStatus from LinkedHashMap to thread-safe maps (e.g.,
ConcurrentHashMap) or by copying their contents into new maps after the
join/cancel block before reading them.
---
Duplicate comments:
In `@src/main/java/com/iemr/inventory/controller/health/HealthController.java`:
- Around line 65-67: The controller now maps overallStatus to HTTP 503 only when
overallStatus equals "DOWN" (using "DOWN".equals(overallStatus)) which is
correct; ensure this remains and add a null-safe check by keeping the
literal-first equals usage for overallStatus in HealthController (reference
overallStatus and healthStatus) and add unit tests that assert responses for
"UP", "DEGRADED", and "DOWN" map to HttpStatus.OK and
HttpStatus.SERVICE_UNAVAILABLE respectively so the behavior is covered by tests.
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 115-123: The health check cancels mysqlFuture/redisFuture but
CompletableFuture.cancel(true) doesn't interrupt the pool threads, so the Redis
PING (referenced at the Redis PING call and redisFuture in HealthService) can
block pool threads; to fix this, enforce a real timeout for the Redis operation
instead of relying on cancel: either configure the Redis client command/socket
timeout (spring.data.redis/Lettuce/Jedis) so the PING returns within a bounded
time, or wrap the Redis PING in a cancellable task that uses a dedicated
timeout-aware client call or explicit timeout logic (e.g., use a client API with
command timeout or run the ping via an executor and use future.get(timeout) and
ensure the Redis client supports aborting the operation); keep the existing
stmt.setQueryTimeout usage for MySQL and apply the same explicit timeout
strategy to the Redis check so redisFuture does not occupy pool threads
indefinitely.
---
Nitpick comments:
In `@src/main/java/com/iemr/inventory/controller/health/HealthController.java`:
- Around line 60-70: In HealthController.checkHealth(), change the initial
logger call from logger.info("Health check endpoint called") to
logger.debug(...) to avoid INFO-level noise from frequent health probes; keep
the existing completion log (logger.debug("Health check completed with status:
{}"...)) as-is so both entry and exit are at DEBUG level and the method behavior
and returned ResponseEntity remain unchanged.
- Around line 72-81: The catch block in HealthController returns a minimal
errorResponse missing "components" and an "error" detail; update the
ResponseEntity returned in the catch of the health-check method to include a
components object (e.g., an empty or minimal Map under "components") and an
"error" field with a sanitized message (not stack trace) alongside "status" and
"timestamp" so the response schema matches the happy path; adjust the Map
construction used for errorResponse and keep the logger.error("Unexpected error
during health check", e) as-is while returning ResponseEntity<>(errorResponse,
HttpStatus.SERVICE_UNAVAILABLE).
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 73-77: The constructor in HealthService currently uses
Executors.newFixedThreadPool(2) which creates non‑daemon, generically named
threads; replace that with a fixed thread pool constructed with a custom
ThreadFactory that sets threads as daemon and gives them a descriptive name
prefix (e.g., "health-check-") so they don't block JVM exit if `@PreDestroy` isn't
invoked and are easier to identify in logs; update the HealthService constructor
to build that ThreadFactory and pass it to Executors.newFixedThreadPool, keeping
executorService as the same field.
- Around line 239-266: performHealthCheck currently mutates the passed-in Map
and returns it, which is misleading because callers (used with
runAsync/Runnable) ignore the return; change the method signature from
Map<String,Object> performHealthCheck(...) to void performHealthCheck(...),
remove the return statement, and keep the existing in-place updates (STATUS_KEY,
"responseTimeMs", SEVERITY_KEY, error/message handling, and
determineSeverity/HealthCheckResult usage) so callers that pass the status map
continue to see the updates without a returned value.
- Around line 189-237: Remove the dead, private deprecated wrappers
checkMySQLHealth() and checkRedisHealth(): they wrap async CompletableFuture
calls to checkMySQLHealthSync and checkRedisHealthSync but are unused since
checkHealth() now calls the sync variants directly; delete both methods
(including their CompletableFuture usage and related timeout/exception handling)
and ensure no remaining references to checkMySQLHealth or checkRedisHealth exist
(leave checkMySQLHealthSync, checkRedisHealthSync, MYSQL_TIMEOUT_SECONDS,
REDIS_TIMEOUT_SECONDS, and executorService intact).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/iemr/inventory/service/health/HealthService.java (2)
251-253:performHealthCheckreturn value is never used — change return type tovoid.The method mutates the
statusmap passed by reference, and all callers invoke it viaCompletableFuture.runAsync(which discards any return value). Returningstatusis superfluous and misleads readers into expecting a meaningful return.♻️ Proposed refactor
- private Map<String, Object> performHealthCheck(String componentName, - Map<String, Object> status, - Supplier<HealthCheckResult> checker) { + private void performHealthCheck(String componentName, + Map<String, Object> status, + Supplier<HealthCheckResult> checker) {And remove both
return status;andreturn status;(lines 278 and 289).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 251 - 253, The performHealthCheck method currently returns Map<String,Object> but its return value is never used (callers invoke it via CompletableFuture.runAsync); change its signature to void (private void performHealthCheck(String componentName, Map<String,Object> status, Supplier<HealthCheckResult> checker), remove the two "return status;" statements inside the method, and ensure callers (which use CompletableFuture.runAsync(() -> performHealthCheck(...))) remain unchanged; this clarifies that the method mutates the passed-in status map instead of producing a value.
201-249: Dead deprecated private methods should be removed, not deprecated.
checkMySQLHealth()(line 202) andcheckRedisHealth()(line 231) areprivatemethods with no call sites in the file —checkHealth()now calls the*Syncvariants directly. Deprecating private methods serves no purpose since they are inaccessible outside the class and there are no consumers to migrate. Keeping them adds maintenance surface and introduces a subtle risk: if either is ever invoked internally, it re-submits to the same shared 2-threadexecutorService, which could cause thread pool exhaustion under concurrentcheckHealth()calls.These should simply be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 201 - 249, Remove the two dead private methods checkMySQLHealth() and checkRedisHealth() (including their `@Deprecated` annotations, javadoc blocks and entire method bodies) since no internal or external call sites exist and they re-submit work to the shared executorService; after removal, run a build/test and delete any now-unused imports/fields (e.g., references only used by those methods such as the methods' javadocs or timeout constants if they become unused) to keep the class clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 117-118: The current use of redisFuture.cancel(true) (and
mysqlFuture.cancel(true)) in checkHealth() is ineffective because
CompletableFuture.cancel does not interrupt the pooled thread; fix by enforcing
a real command-level timeout and ensuring the underlying Redis connection is
closed on timeout: update the Redis health check (e.g., checkRedisHealth()/the
lambda submitted that creates redisFuture) to use the Redis client's
socket/command timeout or client API (Lettuce/Jedis) timeout configuration, or
run the Redis call via an ExecutorService.submit(...) obtaining a
java.util.concurrent.Future and call future.get(timeout, unit) so you can catch
TimeoutException and then explicitly close the Redis connection/resource; remove
reliance on CompletableFuture.cancel(true) and ensure the thread pool size and
task handling won't block subsequent checkHealth() calls.
- Around line 101-102: The mysqlStatus and redisStatus maps are plain
LinkedHashMap and can be written concurrently by performHealthCheck() while the
main thread calls ensurePopulated() and components.put(), causing a data race;
replace the maps with thread-safe implementations (e.g., use
java.util.concurrent.ConcurrentHashMap for mysqlStatus and redisStatus and
import it) so concurrent writes from performHealthCheck() won't race with reads
in ensurePopulated()/components.put(), and ensure any method signatures or local
variable types referencing these maps (mysqlStatus, redisStatus) are updated
accordingly.
---
Nitpick comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 251-253: The performHealthCheck method currently returns
Map<String,Object> but its return value is never used (callers invoke it via
CompletableFuture.runAsync); change its signature to void (private void
performHealthCheck(String componentName, Map<String,Object> status,
Supplier<HealthCheckResult> checker), remove the two "return status;" statements
inside the method, and ensure callers (which use CompletableFuture.runAsync(()
-> performHealthCheck(...))) remain unchanged; this clarifies that the method
mutates the passed-in status map instead of producing a value.
- Around line 201-249: Remove the two dead private methods checkMySQLHealth()
and checkRedisHealth() (including their `@Deprecated` annotations, javadoc blocks
and entire method bodies) since no internal or external call sites exist and
they re-submit work to the shared executorService; after removal, run a
build/test and delete any now-unused imports/fields (e.g., references only used
by those methods such as the methods' javadocs or timeout constants if they
become unused) to keep the class clean.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/inventory/service/health/HealthService.java (1)
225-263: Remove unused methodscheckMySQLHealth()andcheckRedisHealth().These methods (lines 225–263) are dead code—never invoked anywhere in the codebase.
checkHealth()callscheckMySQLHealthSyncandcheckRedisHealthSyncdirectly viaperformHealthCheckinstead. Keeping these unused methods creates unnecessary clutter and potential confusion during maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/inventory/service/health/HealthService.java` around lines 225 - 263, Delete the two unused wrapper methods checkMySQLHealth() and checkRedisHealth() from the HealthService class; they are dead code (never called) because checkHealth() uses checkMySQLHealthSync and checkRedisHealthSync directly via performHealthCheck. Remove both methods (including their CompletableFuture logic and associated timeout/exception handling) and ensure no other code references checkMySQLHealth or checkRedisHealth; leave checkMySQLHealthSync, checkRedisHealthSync and performHealthCheck intact for actual health checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 462-475: The hasDeadlocks(Connection) method currently treats any
historical "LATEST DETECTED DEADLOCK" in the output of "SHOW ENGINE INNODB
STATUS" as active; update hasDeadlocks to parse the timestamp that precedes the
deadlock entry in the innodbStatus string and only return true if that timestamp
is within a configurable recency window (e.g., deadlockRecencySeconds),
otherwise return false; add a config/constant deadlockRecencySeconds, implement
robust parsing of the timestamp format present in the INNODB status (falling
back to previous behavior only if parsing fails), and log parse failures via
logger.debug in the hasDeadlocks method.
- Around line 393-411: The executeAdvancedChecksWithTimeout method causes nested
submissions to the same executor and uses a Connection that may be closed before
the queued task runs; remove executeAdvancedChecksWithTimeout entirely and
change the MySQL health flow to call performAdvancedMySQLChecks(connection)
directly and synchronously from within the MySQL executor task (e.g., from
checkMySQLHealthSync or performHealthCheck("MySQL", ...)), so no additional task
is submitted to executorService and the Connection is used on the same thread;
rely on the per-query JDBC timeouts (stmt.setQueryTimeout) and the existing
outer maxTimeout in checkHealth() to bound runtime and delete any timeout/cancel
logic and associated log branches in executeAdvancedChecksWithTimeout.
---
Duplicate comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 131-132: The LinkedHashMap instances mysqlStatus and redisStatus
are vulnerable to concurrent writes from performHealthCheck while the main
thread may call ensurePopulated after an allOf().get() timeout/cancel; replace
those LinkedHashMap constructions with thread-safe ConcurrentHashMap (or
Collections.newConcurrentMap) so concurrent updates are safe, and ensure any
iteration in ensurePopulated/response building uses the concurrent map snapshot
or safe iteration; update references to mysqlStatus and redisStatus accordingly.
- Around line 147-148: The current use of CompletableFuture.cancel(true)
(mysqlFuture and redisFuture) doesn't interrupt the underlying JDBC/Redis work
and leaves executorService threads blocked; change the async health checks to
use executorService.submit(Callable) (store the returned Future<?> e.g.,
mysqlTask and redisTask) so cancel(true) will attempt to interrupt the worker
thread, and also ensure the JDBC/Redis operations respect interrupts or have
explicit socket/query timeouts (e.g., Statement.setQueryTimeout or client
timeouts) so the interrupted thread can terminate promptly; update HealthService
to cancel those submitted Futures instead of calling cancel on the
CompletableFutures and add interrupt-aware/timeout configuration in the
JDBC/Redis check methods.
---
Nitpick comments:
In `@src/main/java/com/iemr/inventory/service/health/HealthService.java`:
- Around line 225-263: Delete the two unused wrapper methods checkMySQLHealth()
and checkRedisHealth() from the HealthService class; they are dead code (never
called) because checkHealth() uses checkMySQLHealthSync and checkRedisHealthSync
directly via performHealthCheck. Remove both methods (including their
CompletableFuture logic and associated timeout/exception handling) and ensure no
other code references checkMySQLHealth or checkRedisHealth; leave
checkMySQLHealthSync, checkRedisHealthSync and performHealthCheck intact for
actual health checks.
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/inventory/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
|
drtechie
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the Inventory-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Chores