feat(health,version): add health and version endpoints#130
Merged
drtechie merged 8 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#130drtechie merged 8 commits intoPSMRI:mainfrom
drtechie merged 8 commits intoPSMRI:mainfrom
Conversation
Contributor
|
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 /health and /version REST endpoints, a concurrent HealthService performing MySQL and Redis checks with timeouts and throttled diagnostics, updates JWT filter to skip these endpoints, and updates pom.xml to change artifact finalName and add git-commit-id-maven-plugin. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthController
participant HealthService
participant ExecutorService
participant MySQL
participant Redis
Client->>HealthController: GET /health
HealthController->>HealthService: checkHealth()
HealthService->>ExecutorService: submit MySQL check task
HealthService->>ExecutorService: submit Redis check task
par MySQL Check
ExecutorService->>MySQL: SELECT 1 (3s timeout)
MySQL-->>ExecutorService: result / timeout
and Redis Check
ExecutorService->>Redis: PING (3s timeout)
Redis-->>ExecutorService: PONG / timeout
end
ExecutorService-->>HealthService: MySQL HealthCheckResult
ExecutorService-->>HealthService: Redis HealthCheckResult
HealthService->>HealthService: aggregate component statuses -> overall status
HealthService-->>HealthController: health map (timestamp, components, status)
HealthController-->>Client: HTTP 200 (UP/DEGRADED) or 503 (DOWN) with JSON
sequenceDiagram
participant Client
participant VersionController
participant GitPropertiesFile
Client->>VersionController: GET /version
VersionController->>GitPropertiesFile: load git.properties
GitPropertiesFile-->>VersionController: Properties object
VersionController->>VersionController: extract branch, commitHash, version, buildTimestamp
VersionController-->>Client: ResponseEntity<Map> (200 JSON)
Note over VersionController: On exception, populate fields with UNKNOWN_VALUE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/iemr/tm/service/health/HealthService.java (3)
68-71:shutdown()should await termination to let in-flight checks complete.Calling
shutdown()withoutawaitTermination()means the@PreDestroycallback returns immediately, and in-flight health-check tasks may be interrupted mid-connection by the JVM exit.♻️ Suggested fix
`@PreDestroy` public void shutdown() { executorService.shutdown(); + try { + if (!executorService.awaitTermination(5, TimeUnit.SECONDS)) { + executorService.shutdownNow(); + } + } catch (InterruptedException e) { + executorService.shutdownNow(); + Thread.currentThread().interrupt(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 68 - 71, The shutdown() `@PreDestroy` currently calls executorService.shutdown() but returns immediately; update shutdown() to call executorService.shutdown() then awaitTermination with a sensible timeout (e.g., a few seconds) to allow in-flight health checks to complete, handle InterruptedException by restoring the interrupt status and calling executorService.shutdownNow() if needed, and log if the executor did not terminate within the timeout; refer to the shutdown() method and executorService field when making this change.
99-133: Double-timeout: JDBCsetQueryTimeoutandfuture.get()both use 3 s.If the JDBC driver honors the query timeout, it fires at ~3 s. The
future.get()timeout also fires at 3 s. In a tight race the outerTimeoutExceptionmay win, hiding the more informative JDBC timeout message. Consider making the JDBC timeout slightly shorter (e.g.,MYSQL_TIMEOUT_SECONDS - 1) so the inner timeout triggers first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 99 - 133, In checkMySQLHealth adjust the two timeouts so the JDBC driver’s stmt.setQueryTimeout fires before the outer future.get: set the PreparedStatement timeout to something slightly shorter (e.g., setQueryTimeout((int)Math.max(1, MYSQL_TIMEOUT_SECONDS - 1))) and keep future.get(MYSQL_TIMEOUT_SECONDS, TimeUnit.SECONDS) unchanged; update any related logging/messages if needed so the more informative JDBC timeout (thrown inside the CompletableFuture) is observed instead of the outer TimeoutException.
73-97: Health checks run sequentially despite the async machinery — consider parallelizing.
checkMySQLHealth()submits to the executor and immediately blocks onfuture.get(), thencheckRedisHealth()does the same. This means worst-case latency isMYSQL_TIMEOUT + REDIS_TIMEOUT(6 s). Since you already have an executor, you can fire both futures simultaneously and join afterward to cut latency roughly in half.♻️ Sketch: parallel health checks
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); + // Fire both checks in parallel + CompletableFuture<Map<String, Object>> mysqlFuture = CompletableFuture.supplyAsync(() -> { + Map<String, Object> s = new LinkedHashMap<>(); + performHealthCheck("MySQL", s, this::checkMySQLHealth); + return s; + }, executorService); + + CompletableFuture<Map<String, Object>> redisFuture = CompletableFuture.supplyAsync(() -> { + Map<String, Object> s = new LinkedHashMap<>(); + performHealthCheck("Redis", s, this::checkRedisHealth); + return s; + }, executorService); + + components.put("mysql", mysqlFuture.join()); + components.put("redis", redisFuture.join()); response.put("components", components);Note: if you parallelize this way, the inner
checkMySQLHealth/checkRedisHealthmethods would need to run their work on the calling thread (or you'd need a larger pool), since the outer future already occupies a pool thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 73 - 97, The checkHealth method currently calls performHealthCheck sequentially which submits work and immediately blocks; instead submit both health tasks to the executor first (e.g., create two Futures by submitting Callables that invoke checkMySQLHealth and checkRedisHealth or wrappers that call those methods) so both run in parallel, then wait/join both futures and populate the mysql and redis maps with their results; ensure checkMySQLHealth and checkRedisHealth perform their work on the calling thread (or increase the executor pool) so you don't double-occupy pool threads, and keep using performHealthCheck/isHealthy logic to set component status and the overall STATUS_KEY after both futures complete.src/main/java/com/iemr/tm/controller/health/HealthController.java (1)
61-61: Lower log level todebug— health probes typically poll every few seconds.Kubernetes liveness/readiness probes (or similar monitors) will hit
/healthfrequently.logger.infoon every invocation will produce significant log noise in production. The completion log on line 69 already usesdebug.♻️ Suggested 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/tm/controller/health/HealthController.java` at line 61, The health check log at the start of the health endpoint should be lowered from info to debug to avoid noise; locate the HealthController class and the method that handles the /health endpoint (the line with logger.info("Health check endpoint called")) and change that call to logger.debug(...) so it matches the existing debug-level completion log and reduces frequent probe logging.src/main/java/com/iemr/tm/controller/version/VersionController.java (1)
49-67: Consider caching git properties — they are immutable for the lifetime of the process.
git.propertiesis baked into the artifact at build time and never changes at runtime, yetloadGitProperties()re-reads the classpath resource on every call. Loading once at construction (or lazily) avoids repeated I/O on a potentially hot path (e.g., monitoring probes polling/version).♻️ Proposed: load once at construction time
`@RestController` public class VersionController { private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); - private static final String UNKNOWN_VALUE = "unknown"; + private final Map<String, String> versionInfo; + + public VersionController() { + this.versionInfo = loadVersionInfo(); + } `@Operation`(summary = "Get version information") `@GetMapping`(value = "/version", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity<Map<String, String>> versionInformation() { - Map<String, String> response = new LinkedHashMap<>(); - try { - logger.info("version Controller Start"); - Properties gitProperties = loadGitProperties(); - response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); - response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); - response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); - response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); - } catch (Exception e) { - logger.error("Failed to load version information", e); - response.put("buildTimestamp", UNKNOWN_VALUE); - response.put("version", UNKNOWN_VALUE); - response.put("branch", UNKNOWN_VALUE); - response.put("commitHash", UNKNOWN_VALUE); - } - logger.info("version Controller End"); - return ResponseEntity.ok(response); + return ResponseEntity.ok(versionInfo); + } + + private Map<String, String> loadVersionInfo() { + Map<String, String> info = new LinkedHashMap<>(); + try { + Properties gitProperties = loadGitProperties(); + info.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); + info.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); + info.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); + info.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); + } catch (Exception e) { + logger.error("Failed to load version information", e); + info.put("buildTimestamp", UNKNOWN_VALUE); + info.put("version", UNKNOWN_VALUE); + info.put("branch", UNKNOWN_VALUE); + info.put("commitHash", UNKNOWN_VALUE); + } + return Map.copyOf(info); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/controller/version/VersionController.java` around lines 49 - 67, The versionInformation() method repeatedly calls loadGitProperties() on every request; change this to load and cache the Properties once (either in the controller constructor or via a thread-safe lazy init) so subsequent calls reuse the same Properties instance. Add a private field (e.g., cachedGitProperties) and populate it at construction time (or initialize it in a synchronized/volatile lazy getter), preserve the same fallback to UNKNOWN_VALUE on load failure, and update versionInformation() to read from cachedGitProperties instead of calling loadGitProperties() each time; keep logging and exception handling consistent.
🤖 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/tm/service/health/HealthService.java`:
- Around line 135-138: checkRedisHealth currently reports Redis as healthy when
redisTemplate == null which is misleading; update checkRedisHealth in
HealthService so that when redisTemplate is null it does not return an "UP"
health result—either return a HealthCheckResult that indicates a NOT_CONFIGURED
/ distinct status (e.g., healthy=false with details="NOT_CONFIGURED") or return
null and adjust checkHealth to omit Redis from the composite response; locate
checkRedisHealth and the caller checkHealth to implement the chosen behavior and
ensure the response format remains consistent.
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/controller/health/HealthController.java`:
- Line 61: The health check log at the start of the health endpoint should be
lowered from info to debug to avoid noise; locate the HealthController class and
the method that handles the /health endpoint (the line with logger.info("Health
check endpoint called")) and change that call to logger.debug(...) so it matches
the existing debug-level completion log and reduces frequent probe logging.
In `@src/main/java/com/iemr/tm/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method repeatedly calls
loadGitProperties() on every request; change this to load and cache the
Properties once (either in the controller constructor or via a thread-safe lazy
init) so subsequent calls reuse the same Properties instance. Add a private
field (e.g., cachedGitProperties) and populate it at construction time (or
initialize it in a synchronized/volatile lazy getter), preserve the same
fallback to UNKNOWN_VALUE on load failure, and update versionInformation() to
read from cachedGitProperties instead of calling loadGitProperties() each time;
keep logging and exception handling consistent.
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 68-71: The shutdown() `@PreDestroy` currently calls
executorService.shutdown() but returns immediately; update shutdown() to call
executorService.shutdown() then awaitTermination with a sensible timeout (e.g.,
a few seconds) to allow in-flight health checks to complete, handle
InterruptedException by restoring the interrupt status and calling
executorService.shutdownNow() if needed, and log if the executor did not
terminate within the timeout; refer to the shutdown() method and executorService
field when making this change.
- Around line 99-133: In checkMySQLHealth adjust the two timeouts so the JDBC
driver’s stmt.setQueryTimeout fires before the outer future.get: set the
PreparedStatement timeout to something slightly shorter (e.g.,
setQueryTimeout((int)Math.max(1, MYSQL_TIMEOUT_SECONDS - 1))) and keep
future.get(MYSQL_TIMEOUT_SECONDS, TimeUnit.SECONDS) unchanged; update any
related logging/messages if needed so the more informative JDBC timeout (thrown
inside the CompletableFuture) is observed instead of the outer TimeoutException.
- Around line 73-97: The checkHealth method currently calls performHealthCheck
sequentially which submits work and immediately blocks; instead submit both
health tasks to the executor first (e.g., create two Futures by submitting
Callables that invoke checkMySQLHealth and checkRedisHealth or wrappers that
call those methods) so both run in parallel, then wait/join both futures and
populate the mysql and redis maps with their results; ensure checkMySQLHealth
and checkRedisHealth perform their work on the calling thread (or increase the
executor pool) so you don't double-occupy pool threads, and keep using
performHealthCheck/isHealthy logic to set component status and the overall
STATUS_KEY after both futures complete.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/java/com/iemr/tm/service/health/HealthService.java (2)
136-137:ConcurrentHashMapis unnecessary for per-task status maps.Each map is written by exactly one executor thread and read by the main thread only after
future.get(), which already establishes happens-before. A plainnew LinkedHashMap<>()(consistent with the rest of the method) suffices and preserves insertion order.♻️ Proposed fix
- Map<String, Object> mysqlStatus = new ConcurrentHashMap<>(); - Map<String, Object> redisStatus = new ConcurrentHashMap<>(); + Map<String, Object> mysqlStatus = new LinkedHashMap<>(); + Map<String, Object> redisStatus = new LinkedHashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 136 - 137, Replace the unnecessary ConcurrentHashMap instances used for per-task status with a plain insertion-ordered map: change the declarations of mysqlStatus and redisStatus from ConcurrentHashMap to LinkedHashMap (e.g., new LinkedHashMap<>()) in HealthService (the variables mysqlStatus and redisStatus), since each map is written by a single executor thread and read after future.get(), so synchronization is not required and insertion order should be preserved.
105-106:ADVANCED_HEALTH_CHECKS_ENABLEDis a compile-time constant and cannot be toggled without recompiling.The comment acknowledges this as a workaround, but
@Valueworks correctly on instance fields in Spring. Making it configurable avoids having to redeploy to disable expensive advanced checks in a degraded environment.♻️ Proposed fix
- // Advanced health checks enabled - static to avoid Spring property parsing issues - private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true; + `@Value`("${health.advanced-checks.enabled:true}") + private boolean advancedHealthChecksEnabled;Then replace all
ADVANCED_HEALTH_CHECKS_ENABLEDreferences withadvancedHealthChecksEnabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 105 - 106, The ADVANCED_HEALTH_CHECKS_ENABLED is a static compile-time constant; change it to an instance-level configurable property by adding a non-static boolean field advancedHealthChecksEnabled in HealthService wired with `@Value` (with a sensible default like true) and remove/replace all references to ADVANCED_HEALTH_CHECKS_ENABLED with advancedHealthChecksEnabled so the flag can be toggled via Spring properties at runtime; ensure the field is used in the same methods that currently reference ADVANCED_HEALTH_CHECKS_ENABLED and that the class remains Spring-managed so `@Value` is effective.src/main/java/com/iemr/tm/controller/health/HealthController.java (2)
76-79: Error-fallback response is structurally inconsistent with the normal response — missing the"components"key.Consumers that always expect
componentsin the payload will fail to parse the exceptional 503. Adding a minimalcomponentsentry improves predictability.♻️ Proposed fix
- Map<String, Object> errorResponse = Map.of( - "status", "DOWN", - "timestamp", Instant.now().toString() - ); + Map<String, Object> errorResponse = new java.util.LinkedHashMap<>(); + errorResponse.put("timestamp", Instant.now().toString()); + errorResponse.put("components", Map.of()); + errorResponse.put("status", "DOWN");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/controller/health/HealthController.java` around lines 76 - 79, The error fallback in HealthController builds errorResponse without the "components" field, making it structurally different from normal responses; update the errorResponse Map construction (variable errorResponse in HealthController) to include a minimal "components" entry (e.g., an empty Map or the same shape as the normal components payload) alongside "status" and "timestamp" so the 503 payload always contains "components" for consistent consumer parsing.
61-61: Uselogger.debugfor the per-request entry log to avoid INFO-level noise from frequent health probes.Load balancers and monitoring systems typically poll
/healthevery few seconds per instance, flooding logs atINFOlevel.♻️ 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/tm/controller/health/HealthController.java` at line 61, Change the per-request entry log in the HealthController from INFO to DEBUG to avoid INFO-level noise from frequent health probes: locate the health endpoint method in class HealthController where logger.info("Health check endpoint called") is used and replace that call with logger.debug(...) so routine health probe hits are logged at debug level while preserving any remaining INFO-level startup or error logs.
🤖 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/tm/service/health/HealthService.java`:
- Line 112: The fixed two-thread executor in HealthService (executorService
initialized via Executors.newFixedThreadPool(2)) causes concurrent /health
checks to queue and time out; replace that with an unbounded or virtual-thread
pool so per-request MySQL/Redis health tasks don't block each other—if on Java
21+ use a virtual-thread executor, otherwise change to
Executors.newCachedThreadPool(), and update any related imports/initialization
of executorService accordingly.
- Around line 407-412: The SELECT in HealthService (inside the try with
PreparedStatement used by hasLockWaits) compares PROCESSLIST.user to USER(), but
USER() returns 'user@host' while PROCESSLIST.user stores only the username, so
the predicate never matches; change the SQL to compare PROCESSLIST.user to
SUBSTRING_INDEX(USER(), '@', 1) (or otherwise extract the username) and consider
switching the query to performance_schema.processlist for forward compatibility;
update the PreparedStatement creation in the hasLockWaits method (and similarly
hasSlowQueries if applicable) to use the adjusted/replacement table and username
extraction so the filter correctly matches the current DB user.
---
Duplicate comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 217-218: The HealthService health check currently returns new
HealthCheckResult(true, "Redis not configured — skipped", false) when
redisTemplate is null; no code change is needed because the review requirement
to surface a message for the not-configured case has been satisfied—confirm the
HealthService method that checks redisTemplate retains this message and that
HealthCheckResult(...) is used consistently elsewhere.
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/controller/health/HealthController.java`:
- Around line 76-79: The error fallback in HealthController builds errorResponse
without the "components" field, making it structurally different from normal
responses; update the errorResponse Map construction (variable errorResponse in
HealthController) to include a minimal "components" entry (e.g., an empty Map or
the same shape as the normal components payload) alongside "status" and
"timestamp" so the 503 payload always contains "components" for consistent
consumer parsing.
- Line 61: Change the per-request entry log in the HealthController from INFO to
DEBUG to avoid INFO-level noise from frequent health probes: locate the health
endpoint method in class HealthController where logger.info("Health check
endpoint called") is used and replace that call with logger.debug(...) so
routine health probe hits are logged at debug level while preserving any
remaining INFO-level startup or error logs.
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 136-137: Replace the unnecessary ConcurrentHashMap instances used
for per-task status with a plain insertion-ordered map: change the declarations
of mysqlStatus and redisStatus from ConcurrentHashMap to LinkedHashMap (e.g.,
new LinkedHashMap<>()) in HealthService (the variables mysqlStatus and
redisStatus), since each map is written by a single executor thread and read
after future.get(), so synchronization is not required and insertion order
should be preserved.
- Around line 105-106: The ADVANCED_HEALTH_CHECKS_ENABLED is a static
compile-time constant; change it to an instance-level configurable property by
adding a non-static boolean field advancedHealthChecksEnabled in HealthService
wired with `@Value` (with a sensible default like true) and remove/replace all
references to ADVANCED_HEALTH_CHECKS_ENABLED with advancedHealthChecksEnabled so
the flag can be toggled via Spring properties at runtime; ensure the field is
used in the same methods that currently reference ADVANCED_HEALTH_CHECKS_ENABLED
and that the class remains Spring-managed so `@Value` is effective.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tm/service/health/HealthService.java`:
- Line 107: The constant ADVANCED_HEALTH_CHECKS_ENABLED is a compile-time true
and makes the runtime guard (the if (!ADVANCED_HEALTH_CHECKS_ENABLED) return
false; check) unreachable; remove the constant and the dead branch or replace
the constant with a runtime/config flag (e.g., read from application properties
or an env var) so the guard becomes meaningful. Specifically, delete
ADVANCED_HEALTH_CHECKS_ENABLED and the guarded return, or change
ADVANCED_HEALTH_CHECKS_ENABLED into a non-final field or config lookup and wire
it into the HealthService initialization so the existing guard that references
ADVANCED_HEALTH_CHECKS_ENABLED behaves correctly.
- Around line 407-433: The method
handleAdvancedChecksFuture(Future<AdvancedCheckResult> future) is missing its
final closing brace; add a single '}' to close the method after the last
catch(Exception ex) block (i.e., immediately after the existing catch block that
logs "Advanced MySQL checks failed") so the next method/field declarations are
outside handleAdvancedChecksFuture; locate the method by its name and references
to ADVANCED_CHECKS_TIMEOUT_MS and logger to find the correct spot.
---
Duplicate comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Line 113: The fixed-size pool created in HealthService (executorService =
Executors.newFixedThreadPool(6)) limits concurrent /health handling and can
starve callers; replace that allocation with a virtual-thread per-task executor
on Java 21+ (use Executors.newVirtualThreadPerTaskExecutor()) or, if supporting
Java <21, switch to an unbounded cached pool (Executors.newCachedThreadPool())
so HealthService.executorService can handle many concurrent tasks without the
6-thread ceiling.
- Around line 469-505: Update the two queries in hasLockWaits and hasSlowQueries
to use performance_schema.processlist instead of the deprecated
INFORMATION_SCHEMA.PROCESSLIST; locate the PreparedStatement constructions
inside methods hasLockWaits(Connection) and hasSlowQueries(Connection) and
perform a direct
s/INFORMATION_SCHEMA.processlist/performance_schema.processlist/ substitution
while preserving the rest of the SQL, parameter binding (stmt.setInt(1, 10)),
query timeout and result handling logic.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/com/iemr/tm/service/health/HealthService.java (3)
113-113: Fixed pool of 6 still causes starvation at 4+ concurrent/healthcallers.Each
/healthrequest consumes 2executorServiceslots (MySQL + Redis tasks). With 6 threads, the 4th concurrent caller's tasks queue behind earlier requests, starting the 4-second aggregate deadline while the tasks are still waiting. The pool was bumped from 2 → 6, which helps but doesn't eliminate the problem. The recommended fix is a virtual-thread executor (Java 21+) or an unbounded cached pool:♻️ Proposed fix
- this.executorService = Executors.newFixedThreadPool(6); + // Java 21+: lightweight, per-request concurrency with no fixed cap + this.executorService = Executors.newVirtualThreadPerTaskExecutor(); + // Java < 21 alternative: Executors.newCachedThreadPool()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` at line 113, The fixed-size pool in HealthService (executorService created via Executors.newFixedThreadPool) causes task starvation because each /health request submits two tasks (MySQL + Redis); replace the fixed pool with an unbounded/cached or virtual-thread executor so requests don't block: update the executorService initialization (where Executors.newFixedThreadPool(6) is used) to use either a virtual-thread-per-task executor (Executors.newVirtualThreadPerTaskExecutor() on Java 21+) or Executors.newCachedThreadPool() so each health check’s subtasks can run without queuing and avoid aggregate-deadline delays.
107-107: Dead code —ADVANCED_HEALTH_CHECKS_ENABLEDguard is still unreachable.
private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = trueis a compile-time constant; theif (!ADVANCED_HEALTH_CHECKS_ENABLED)branch on Lines 360–362 can never execute. Remove the constant and the dead branch, or replace it with a runtime/config flag (e.g., read fromapplication.properties) to make the guard meaningful.♻️ Proposed cleanup
- // Advanced checks always enabled - private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true;private boolean performAdvancedMySQLChecksWithThrottle() { - if (!ADVANCED_HEALTH_CHECKS_ENABLED) { - return false; // Advanced checks disabled - } - long currentTime = System.currentTimeMillis();Also applies to: 359-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` at line 107, The class-level compile-time constant ADVANCED_HEALTH_CHECKS_ENABLED in HealthService makes the runtime guard unreachable; either remove the constant and delete the dead if-block that checks it (the branch currently unreachable where the code returns early), or convert the flag to a runtime-config boolean (e.g., inject via `@Value`("${health.advanced.enabled:false}") or read from Environment/ApplicationProperties) and replace usages of ADVANCED_HEALTH_CHECKS_ENABLED with that injected field so the if (!ADVANCED_HEALTH_CHECKS_ENABLED) branch in HealthService becomes meaningful at runtime.
471-476:INFORMATION_SCHEMA.PROCESSLISTis deprecated — migrate toperformance_schema.processlist.Both
hasLockWaitsandhasSlowQueriesqueryINFORMATION_SCHEMA.PROCESSLIST, which is deprecated in MySQL 8 and subject to removal. The recommended replacement isperformance_schema.processlist.♻️ Proposed migration for both methods
- "SELECT COUNT(*) FROM INFORMATION_SCHEMA.PROCESSLIST " + + "SELECT COUNT(*) FROM performance_schema.processlist " + "WHERE (state = 'Waiting for table metadata lock' " + " OR state = 'Waiting for row lock' " + " OR state = 'Waiting for lock') " + "AND user = SUBSTRING_INDEX(USER(), '@', 1)")) {- "SELECT COUNT(*) FROM INFORMATION_SCHEMA.PROCESSLIST " + + "SELECT COUNT(*) FROM performance_schema.processlist " + "WHERE command != 'Sleep' AND time > ? AND user NOT IN ('event_scheduler', 'system user')")) {Also applies to: 491-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 471 - 476, Update both hasLockWaits and hasSlowQueries to use performance_schema.processlist instead of INFORMATION_SCHEMA.PROCESSLIST by changing the SQL string(s) to reference performance_schema.processlist (the column names used—state, user, etc.—are the same), and apply the same replacement for the duplicate occurrence around the other query (lines noted in the review). Ensure the PreparedStatement SQL in methods hasLockWaits and hasSlowQueries uses "performance_schema.processlist" in place of "INFORMATION_SCHEMA.PROCESSLIST" so the queries work on MySQL 8+.
🧹 Nitpick comments (2)
src/main/java/com/iemr/tm/service/health/HealthService.java (2)
136-138:advancedCheckExecutorisshutdownNow()'d without waiting, unlikeexecutorService.An in-flight advanced check (up to 500 ms) will be abruptly interrupted with no drain window. For consistency with
executorService, apply a briefawaitTerminationbefore escalating toshutdownNow:♻️ Proposed fix
if (advancedCheckExecutor != null && !advancedCheckExecutor.isShutdown()) { - advancedCheckExecutor.shutdownNow(); + advancedCheckExecutor.shutdown(); + try { + if (!advancedCheckExecutor.awaitTermination(1, TimeUnit.SECONDS)) { + advancedCheckExecutor.shutdownNow(); + } + } catch (InterruptedException e) { + advancedCheckExecutor.shutdownNow(); + Thread.currentThread().interrupt(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 136 - 138, The advancedCheckExecutor is being abruptly shutdown with shutdownNow() without the graceful wait used for executorService; change the shutdown sequence for advancedCheckExecutor to mirror executorService by first calling advancedCheckExecutor.shutdown(), then awaitTermination for a short timeout (e.g., 500 ms) and only call advancedCheckExecutor.shutdownNow() if awaitTermination returns false or is interrupted, ensuring you handle InterruptedException as done for executorService.
558-563: Silent catch inevaluatePoolMetricsswallows JMX attribute errors without any trace.Add at least a
logger.debugso that transient JMX failures are observable in diagnostic logs.♻️ Proposed fix
} catch (Exception e) { - // Continue to next MBean + logger.debug("Could not read pool metrics from MBean {}: {}", objectName, e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 558 - 563, The catch in evaluatePoolMetrics silently swallows exceptions from JMX attribute reads; update the catch block to log the exception at debug level so transient JMX failures are visible. Specifically, inside evaluatePoolMetrics' catch (Exception e) for the MBean attribute inspection, call logger.debug with a concise message that includes context (e.g., the MBean name/ObjectName and attribute being read) and pass the exception (e) so stack/causes are recorded; keep behavior otherwise unchanged so the method still returns false on error.
🤖 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/tm/service/health/HealthService.java`:
- Around line 219-232: checkMySQLHealthSync currently relies on
dataSource.getConnection() which can block up to the Tomcat JDBC pool's maxWait
(default 30s) and violate the 4s health deadline; set a short max-wait for the
health DataSource so connection acquisition respects the MYSQL_TIMEOUT_SECONDS
(e.g. 3000ms) or use a dedicated DataSource for health checks with that
max-wait. Locate uses of dataSource.getConnection() and
stmt.setQueryTimeout(...) in checkMySQLHealthSync and update configuration (e.g.
spring.datasource.tomcat.max-wait) or create/configure a dedicated health
DataSource with max-wait ~3000 to ensure getConnection() returns or fails within
the health timeout.
---
Duplicate comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Line 113: The fixed-size pool in HealthService (executorService created via
Executors.newFixedThreadPool) causes task starvation because each /health
request submits two tasks (MySQL + Redis); replace the fixed pool with an
unbounded/cached or virtual-thread executor so requests don't block: update the
executorService initialization (where Executors.newFixedThreadPool(6) is used)
to use either a virtual-thread-per-task executor
(Executors.newVirtualThreadPerTaskExecutor() on Java 21+) or
Executors.newCachedThreadPool() so each health check’s subtasks can run without
queuing and avoid aggregate-deadline delays.
- Line 107: The class-level compile-time constant ADVANCED_HEALTH_CHECKS_ENABLED
in HealthService makes the runtime guard unreachable; either remove the constant
and delete the dead if-block that checks it (the branch currently unreachable
where the code returns early), or convert the flag to a runtime-config boolean
(e.g., inject via `@Value`("${health.advanced.enabled:false}") or read from
Environment/ApplicationProperties) and replace usages of
ADVANCED_HEALTH_CHECKS_ENABLED with that injected field so the if
(!ADVANCED_HEALTH_CHECKS_ENABLED) branch in HealthService becomes meaningful at
runtime.
- Around line 471-476: Update both hasLockWaits and hasSlowQueries to use
performance_schema.processlist instead of INFORMATION_SCHEMA.PROCESSLIST by
changing the SQL string(s) to reference performance_schema.processlist (the
column names used—state, user, etc.—are the same), and apply the same
replacement for the duplicate occurrence around the other query (lines noted in
the review). Ensure the PreparedStatement SQL in methods hasLockWaits and
hasSlowQueries uses "performance_schema.processlist" in place of
"INFORMATION_SCHEMA.PROCESSLIST" so the queries work on MySQL 8+.
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 136-138: The advancedCheckExecutor is being abruptly shutdown with
shutdownNow() without the graceful wait used for executorService; change the
shutdown sequence for advancedCheckExecutor to mirror executorService by first
calling advancedCheckExecutor.shutdown(), then awaitTermination for a short
timeout (e.g., 500 ms) and only call advancedCheckExecutor.shutdownNow() if
awaitTermination returns false or is interrupted, ensuring you handle
InterruptedException as done for executorService.
- Around line 558-563: The catch in evaluatePoolMetrics silently swallows
exceptions from JMX attribute reads; update the catch block to log the exception
at debug level so transient JMX failures are visible. Specifically, inside
evaluatePoolMetrics' catch (Exception e) for the MBean attribute inspection,
call logger.debug with a concise message that includes context (e.g., the MBean
name/ObjectName and attribute being read) and pass the exception (e) so
stack/causes are recorded; keep behavior otherwise unchanged so the method still
returns false on error.
DurgaPrasad-54
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/java/com/iemr/tm/service/health/HealthService.java (4)
411-447:INFORMATION_SCHEMA.PROCESSLISTis deprecated in MySQL 8.0 and subject to removal in a future release.Both
hasLockWaitsandhasSlowQueriesquery this table. The recommended replacement isperformance_schema.processlist. This was raised in a prior review and remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 411 - 447, Change both hasLockWaits and hasSlowQueries to query performance_schema.processlist instead of INFORMATION_SCHEMA.PROCESSLIST: update the SQL strings in hasLockWaits and hasSlowQueries to use performance_schema.processlist (keeping the same WHERE clauses and parameters), ensure you still set the 2s query timeout and parameters (time parameter in hasSlowQueries), and keep the same result handling; also update the surrounding code/comments/log messages if necessary to reflect the new source (performance_schema.processlist) and verify the DB user has permission to read performance_schema.
90-90:ADVANCED_HEALTH_CHECKS_ENABLEDis a compile-timetrueconstant — the guard at lines 332–334 is unreachable dead code.This issue was flagged in a prior review and remains unresolved. Either remove the constant and the dead branch or replace it with a runtime/config flag (e.g., read from application properties) so the guard becomes meaningful.
Also applies to: 332-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` at line 90, The ADVANCED_HEALTH_CHECKS_ENABLED constant in HealthService is a compile-time true which makes the conditional guard around the advanced health-check logic unreachable; replace it with a runtime-config boolean or remove the dead branch: either (A) delete the ADVANCED_HEALTH_CHECKS_ENABLED constant and remove the guarded code path (the advanced health-check block) from the HealthService class, or (B) make it configurable by adding a private boolean field (e.g., advancedHealthChecksEnabled) populated from application properties or env (use `@Value`("${app.advancedHealthChecks:false}") or a ConfigProperties bean) and replace all uses of ADVANCED_HEALTH_CHECKS_ENABLED with that field so the guard around the advanced health-check method(s) becomes effective (ensure to update any constructors or autowired config to inject the property).
194-211: BothdataSource.getConnection()calls are bounded only by the connection pool'smaxWait(default ≈ 30 s), not the 4-second health deadline.The prior review raised this for
checkMySQLHealthSync;performAdvancedMySQLChecksWithThrottle(line 361) has the same exposure. If the DB host drops packets, either call can pin the executor thread well beyond the aggregate deadline. Mitigate by configuring a shortmaxWait(≤ 3 s) on this DataSource or by using a dedicated health DataSource.Also applies to: 361-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 194 - 211, checkMySQLHealthSync and performAdvancedMySQLChecksWithThrottle currently call dataSource.getConnection() which can block up to the pool's maxWait (≈30s) and exceed the 4s health deadline; fix by ensuring connection acquisition is bounded: either configure the DataSource used by these health checks with a short maxWait (≤3s) or create and use a dedicated health DataSource (e.g., healthDataSource) with aggressive maxWait/connection acquisition settings, then update checkMySQLHealthSync and performAdvancedMySQLChecksWithThrottle to use that DataSource so connection acquisition cannot pin the executor beyond the health deadline.
96-96: Fixed thread pool still risks concurrent-probe starvation.Increasing the pool from 2 to 6 delays but does not eliminate the issue: three or more concurrent
/healthprobes will still queue tasks and false-DOWN under load. The prior recommendation to switch to a virtual-thread or cached pool remains applicable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` at line 96, The fixed-size executorService created in HealthService via Executors.newFixedThreadPool(6) can still cause probe-task queuing and false DOWN responses; replace it with a non-bounded/thread-per-task executor such as Executors.newCachedThreadPool() or, if running on a Java version that supports Project Loom, Executors.newVirtualThreadPerTaskExecutor() to ensure healthProbe tasks in HealthService do not block each other; update the executorService initialization and any shutdown logic accordingly (search for executorService and the constructor/initialization in HealthService to locate the change).
🧹 Nitpick comments (1)
src/main/java/com/iemr/tm/service/health/HealthService.java (1)
307-308: Redundant|| SEVERITY_CRITICAL.equals(severity)incomputeOverallStatus.
SEVERITY_CRITICALis set exclusively whenSTATUS_DOWNis also set (indetermineSeverity,performHealthCheck's catch block, andensurePopulated). The second condition is never independently true, making the OR misleading.♻️ Proposed cleanup
- if (STATUS_DOWN.equals(status) || SEVERITY_CRITICAL.equals(severity)) { + if (STATUS_DOWN.equals(status)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 307 - 308, The check in computeOverallStatus uses "if (STATUS_DOWN.equals(status) || SEVERITY_CRITICAL.equals(severity))" but SEVERITY_CRITICAL is only ever set when STATUS_DOWN is set, so remove the redundant "|| SEVERITY_CRITICAL.equals(severity)" term; update computeOverallStatus to only test STATUS_DOWN.equals(status) (or otherwise consolidate logic) and ensure no other callers rely on the secondary check; reference computeOverallStatus and corroborate with determineSeverity, performHealthCheck, and ensurePopulated to keep behavior consistent.
🤖 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/tm/service/health/HealthService.java`:
- Around line 405-408: The catch block in HealthService (surrounding the call to
performAdvancedMySQLChecks) currently treats any Exception as a degraded state
by returning new AdvancedCheckResult(true); change this to fail-open: log the
exception details (using logger.warn or logger.debug with the exception) and
return new AdvancedCheckResult(false) so transient errors don’t mark the service
DEGRADED for the throttle window; keep references to performAdvancedMySQLChecks
and AdvancedCheckResult so reviewers can find and update the catch block
accordingly.
---
Duplicate comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 411-447: Change both hasLockWaits and hasSlowQueries to query
performance_schema.processlist instead of INFORMATION_SCHEMA.PROCESSLIST: update
the SQL strings in hasLockWaits and hasSlowQueries to use
performance_schema.processlist (keeping the same WHERE clauses and parameters),
ensure you still set the 2s query timeout and parameters (time parameter in
hasSlowQueries), and keep the same result handling; also update the surrounding
code/comments/log messages if necessary to reflect the new source
(performance_schema.processlist) and verify the DB user has permission to read
performance_schema.
- Line 90: The ADVANCED_HEALTH_CHECKS_ENABLED constant in HealthService is a
compile-time true which makes the conditional guard around the advanced
health-check logic unreachable; replace it with a runtime-config boolean or
remove the dead branch: either (A) delete the ADVANCED_HEALTH_CHECKS_ENABLED
constant and remove the guarded code path (the advanced health-check block) from
the HealthService class, or (B) make it configurable by adding a private boolean
field (e.g., advancedHealthChecksEnabled) populated from application properties
or env (use `@Value`("${app.advancedHealthChecks:false}") or a ConfigProperties
bean) and replace all uses of ADVANCED_HEALTH_CHECKS_ENABLED with that field so
the guard around the advanced health-check method(s) becomes effective (ensure
to update any constructors or autowired config to inject the property).
- Around line 194-211: checkMySQLHealthSync and
performAdvancedMySQLChecksWithThrottle currently call dataSource.getConnection()
which can block up to the pool's maxWait (≈30s) and exceed the 4s health
deadline; fix by ensuring connection acquisition is bounded: either configure
the DataSource used by these health checks with a short maxWait (≤3s) or create
and use a dedicated health DataSource (e.g., healthDataSource) with aggressive
maxWait/connection acquisition settings, then update checkMySQLHealthSync and
performAdvancedMySQLChecksWithThrottle to use that DataSource so connection
acquisition cannot pin the executor beyond the health deadline.
- Line 96: The fixed-size executorService created in HealthService via
Executors.newFixedThreadPool(6) can still cause probe-task queuing and false
DOWN responses; replace it with a non-bounded/thread-per-task executor such as
Executors.newCachedThreadPool() or, if running on a Java version that supports
Project Loom, Executors.newVirtualThreadPerTaskExecutor() to ensure healthProbe
tasks in HealthService do not block each other; update the executorService
initialization and any shutdown logic accordingly (search for executorService
and the constructor/initialization in HealthService to locate the change).
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 307-308: The check in computeOverallStatus uses "if
(STATUS_DOWN.equals(status) || SEVERITY_CRITICAL.equals(severity))" but
SEVERITY_CRITICAL is only ever set when STATUS_DOWN is set, so remove the
redundant "|| SEVERITY_CRITICAL.equals(severity)" term; update
computeOverallStatus to only test STATUS_DOWN.equals(status) (or otherwise
consolidate logic) and ensure no other callers rely on the secondary check;
reference computeOverallStatus and corroborate with determineSeverity,
performHealthCheck, and ensurePopulated to keep behavior consistent.
…lse degraded state
|
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 TM-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes