feat(health,version): add health and version endpoints#66
Merged
drtechie merged 2 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#66drtechie merged 2 commits intoPSMRI:mainfrom
drtechie merged 2 commits intoPSMRI:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds health monitoring: a new HealthService (concurrent MySQL and Redis checks, diagnostics, throttling) and HealthController GET /health; refactors VersionController to return JSON from git.properties; exempts /health and /version from JWT filter; adds git-commit-id-maven-plugin entries to pom.xml (duplicated). Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant HC as HealthController
participant HS as HealthService
participant EX as ExecutorService
participant DB as MySQL
participant RS as Redis
Client->>HC: GET /health
HC->>HS: checkHealth()
HS->>EX: submit MySQL check
HS->>EX: submit Redis check
par Parallel checks
EX->>DB: basic connectivity (SELECT 1)
DB-->>EX: response
EX->>DB: advanced diagnostics (locks, slow queries, pool)
DB-->>EX: diagnostics
EX->>RS: PING
RS-->>EX: PONG
end
EX-->>HS: component results
HS->>HS: aggregate statuses (UP/DEGRADED/DOWN)
HS-->>HC: health map
HC-->>Client: ResponseEntity (200 or 503)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🧹 Nitpick comments (5)
src/main/java/com/iemr/helpline104/controller/health/HealthController.java (1)
60-83: Consider reducing log level for the health check entry point.Line 61 logs at
INFOon every health check call. Health endpoints are often polled every few seconds by load balancers and monitoring systems, which will generate significant log noise. Consider usingDEBUGfor the entry log and keepingINFO/WARNfor anomalous results only.Suggested change
- 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/helpline104/controller/health/HealthController.java` around lines 60 - 83, The health endpoint logs each invocation at INFO in HealthController.checkHealth which creates noisy logs; change the initial logger.info("Health check endpoint called") to logger.debug(...) so routine polls are logged at DEBUG level, and retain the existing logger.info/warn/error usage for anomalous results or exceptions (i.e., keep the debug entry log but leave the status-based logging and exception logging as-is).src/main/java/com/iemr/helpline104/controller/version/VersionController.java (1)
69-78: Consider caching git properties at construction time.
git.propertiesis static build-time data. Loading it from the classpath on every request is wasteful (albeit harmless for a low-traffic endpoint). You could load once in the constructor or in a@PostConstructand store the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/controller/version/VersionController.java` around lines 69 - 78, The loadGitProperties method currently reads git.properties on each call; instead, add a private Properties field (e.g., gitProperties) to VersionController and initialize it once at construction time (either in the class constructor or a `@PostConstruct` method) by calling loadGitProperties or inlining its logic; handle IOException during init (log or rethrow as an unchecked exception) so subsequent requests use the cached gitProperties field rather than reloading from the classpath each time, and update any methods that call loadGitProperties to use the cached gitProperties field.src/main/java/com/iemr/helpline104/service/health/HealthService.java (3)
193-213: Advanced checks reuse the health-check connection — be mindful of timeout budget.
performAdvancedMySQLChecksWithThrottleruns on the sameConnectionobtained incheckMySQLHealthSync, within the outer try-with-resources. TheSELECT 1has a 3-second query timeout, but the advanced checks each set their own 2-second timeouts. If the advanced checks aren't throttled (i.e., cache miss), the total connection hold time can exceed the 3-secondMYSQL_TIMEOUT_SECONDS, potentially causing the outercheckHealth()deadline to expire. This is mitigated by the 30-second throttle cache but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 193 - 213, checkMySQLHealthSync currently passes the same Connection to performAdvancedMySQLChecksWithThrottle which can exceed the outer SELECT 1's MYSQL_TIMEOUT_SECONDS; change the implementation so that performAdvancedMySQLChecksWithThrottle uses its own DB Connection (i.e., open a new connection inside that method or have it obtain one from the DataSource) so the outer try-with-resources' short query timeout isn't consumed by the advanced checks, and ensure the new connection is properly closed; alternatively, if you prefer to keep a single call site, increase the outer MYSQL_TIMEOUT_SECONDS before invoking advanced checks and restore it afterwards—but prefer the separate-connection approach for isolation and predictable timeouts.
107-112: Fixed thread pool of size 2 may bottleneck under concurrent health check requests.Since
checkHealth()submits exactly 2 tasks per invocation, concurrent callers will queue behind each other on this pool. Under aggressive polling (e.g., multiple load balancers), this could cause cascading timeouts. Consider using a cached thread pool or a slightly larger pool, or — preferably — adding rate-limiting/caching at the controller level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 107 - 112, The fixed-size executor in the HealthService constructor (executorService = Executors.newFixedThreadPool(2)) will bottleneck because checkHealth() submits two tasks per call; change this to a scalable executor (for example use Executors.newCachedThreadPool() or create a configurable pool via a constructor parameter/DI so pool size can be tuned) and/or accept an ExecutorService in the HealthService constructor so callers can provide a shared, appropriately sized executor; update the constructor signature and initialization (or configuration binding) and ensure existing checkHealth() continues to submit tasks to the new executorService.
334-372: Throttle usescurrentTimecaptured before acquiring the write lock — minor staleness.
currentTimeis captured at line 339 before the read lock, but is reused at line 365 (lastAdvancedCheckTime = currentTime) after the write lock is acquired. If there's contention, this timestamp could be stale by the time the write lock is obtained, slightly shortening the effective throttle window. This is a minor timing inaccuracy that's unlikely to cause issues in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 334 - 372, The throttle timestamp is captured in performAdvancedMySQLChecksWithThrottle as currentTime before locking and then reused after acquiring the write lock, which can make the cached timestamp slightly stale; fix by re-reading System.currentTimeMillis() inside the write lock (e.g., update currentTime after acquiring advancedCheckLock.writeLock()) and use that refreshed value for the double-check comparison and when assigning lastAdvancedCheckTime, keeping references to cachedAdvancedCheckResult, ADVANCED_CHECKS_THROTTLE_SECONDS, and advancedCheckLock unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 390-408: The git-commit-id-maven-plugin configuration is missing
generateGitPropertiesFile, so git.properties is not produced and
VersionController.loadGitProperties() ends up returning "unknown"; update the
plugin block for artifactId git-commit-id-maven-plugin (version 5.0.0) to
include <generateGitPropertiesFile>true</generateGitPropertiesFile> in its
<configuration> so the git.properties file is generated onto the classpath and
VersionController.loadGitProperties() can read the git metadata.
In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java`:
- Around line 425-454: The hasDeadlocks method currently treats any presence of
"LATEST DETECTED DEADLOCK" in innodbStatus as a live deadlock; instead parse the
timestamp from the InnoDB status text (variable innodbStatus) within
hasDeadlocks and only return true if the parsed deadlock time is within a recent
window (e.g., last 5 minutes). Use a regex to extract the timestamp line that
accompanies the LATEST DETECTED DEADLOCK block, normalize the parsed time to the
server timezone or assume UTC, compare against Instant.now() (or
System.currentTimeMillis()) with a configurable threshold, and only set a
degraded result when within that threshold; keep the existing
permission-fallback behavior (deadlockCheckDisabled) and ensure any parsing
failures fall back to treating it as non-degraded (return false) while logging a
debug message.
- Around line 155-166: The generic exception catch in HealthService currently
logs the error but fails to cancel redisFuture, leaving an orphaned task; update
the catch (Exception e) block (where mysqlFuture and redisFuture are referenced)
to cancel redisFuture as well (call redisFuture.cancel(true)) — mirror the
cancellation behavior used in the TimeoutException/InterruptedException handlers
so both mysqlFuture and redisFuture are cancelled when any exception (e.g.,
ExecutionException) occurs; keep the existing logging of the error message and
ensure ensurePopulated behavior remains unchanged.
---
Nitpick comments:
In `@src/main/java/com/iemr/helpline104/controller/health/HealthController.java`:
- Around line 60-83: The health endpoint logs each invocation at INFO in
HealthController.checkHealth which creates noisy logs; change the initial
logger.info("Health check endpoint called") to logger.debug(...) so routine
polls are logged at DEBUG level, and retain the existing logger.info/warn/error
usage for anomalous results or exceptions (i.e., keep the debug entry log but
leave the status-based logging and exception logging as-is).
In
`@src/main/java/com/iemr/helpline104/controller/version/VersionController.java`:
- Around line 69-78: The loadGitProperties method currently reads git.properties
on each call; instead, add a private Properties field (e.g., gitProperties) to
VersionController and initialize it once at construction time (either in the
class constructor or a `@PostConstruct` method) by calling loadGitProperties or
inlining its logic; handle IOException during init (log or rethrow as an
unchecked exception) so subsequent requests use the cached gitProperties field
rather than reloading from the classpath each time, and update any methods that
call loadGitProperties to use the cached gitProperties field.
In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java`:
- Around line 193-213: checkMySQLHealthSync currently passes the same Connection
to performAdvancedMySQLChecksWithThrottle which can exceed the outer SELECT 1's
MYSQL_TIMEOUT_SECONDS; change the implementation so that
performAdvancedMySQLChecksWithThrottle uses its own DB Connection (i.e., open a
new connection inside that method or have it obtain one from the DataSource) so
the outer try-with-resources' short query timeout isn't consumed by the advanced
checks, and ensure the new connection is properly closed; alternatively, if you
prefer to keep a single call site, increase the outer MYSQL_TIMEOUT_SECONDS
before invoking advanced checks and restore it afterwards—but prefer the
separate-connection approach for isolation and predictable timeouts.
- Around line 107-112: The fixed-size executor in the HealthService constructor
(executorService = Executors.newFixedThreadPool(2)) will bottleneck because
checkHealth() submits two tasks per call; change this to a scalable executor
(for example use Executors.newCachedThreadPool() or create a configurable pool
via a constructor parameter/DI so pool size can be tuned) and/or accept an
ExecutorService in the HealthService constructor so callers can provide a
shared, appropriately sized executor; update the constructor signature and
initialization (or configuration binding) and ensure existing checkHealth()
continues to submit tasks to the new executorService.
- Around line 334-372: The throttle timestamp is captured in
performAdvancedMySQLChecksWithThrottle as currentTime before locking and then
reused after acquiring the write lock, which can make the cached timestamp
slightly stale; fix by re-reading System.currentTimeMillis() inside the write
lock (e.g., update currentTime after acquiring advancedCheckLock.writeLock())
and use that refreshed value for the double-check comparison and when assigning
lastAdvancedCheckTime, keeping references to cachedAdvancedCheckResult,
ADVANCED_CHECKS_THROTTLE_SECONDS, and advancedCheckLock unchanged.
src/main/java/com/iemr/helpline104/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/helpline104/service/health/HealthService.java
Outdated
Show resolved
Hide resolved
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/main/java/com/iemr/helpline104/service/health/HealthService.java (4)
99-104: Thread pool size is hardcoded.
Executors.newFixedThreadPool(6)is hardcoded, but the pool only ever runs two concurrent tasks (MySQL + Redis checks). A pool of 2 would suffice and avoid holding unnecessary threads. If more components are added later, it can be bumped.Suggested change
- this.executorService = Executors.newFixedThreadPool(6); + this.executorService = Executors.newFixedThreadPool(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 99 - 104, The constructor in HealthService is creating an executorService with a hardcoded size of 6 (Executors.newFixedThreadPool(6)) even though only two concurrent tasks are used; change this to a pool size of 2 or make it configurable. Update the HealthService constructor to initialize executorService with Executors.newFixedThreadPool(2) or fetch the size from a configuration/constant (e.g., THREAD_POOL_SIZE) so the pool is small by default but can be increased later without code changes; reference the HealthService constructor and executorService/Executors.newFixedThreadPool(...) when making the change.
359-391: Thundering herd possible when the throttle window expires.After the write-lock double-check (line 363) releases the lock at line 368, multiple concurrent threads can each pass the stale check and all proceed to execute the expensive DB queries simultaneously. The DB I/O was intentionally moved outside the write lock (per the commit message), but this creates a burst of redundant queries.
A lightweight fix: use an
AtomicBooleanas a "check-in-progress" flag so that only the first thread performs the DB I/O while others return the stale cached result.Sketch using AtomicBoolean guard
+ private final java.util.concurrent.atomic.AtomicBoolean advancedCheckInProgress = new java.util.concurrent.atomic.AtomicBoolean(false); + private boolean performAdvancedMySQLChecksWithThrottle() { if (!ADVANCED_HEALTH_CHECKS_ENABLED) { return false; } long currentTime = System.currentTimeMillis(); advancedCheckLock.readLock().lock(); try { if (cachedAdvancedCheckResult != null && (currentTime - lastAdvancedCheckTime) < ADVANCED_CHECKS_THROTTLE_SECONDS * 1000) { return cachedAdvancedCheckResult.isDegraded; } } finally { advancedCheckLock.readLock().unlock(); } - advancedCheckLock.writeLock().lock(); - try { - if (cachedAdvancedCheckResult != null && - (currentTime - lastAdvancedCheckTime) < ADVANCED_CHECKS_THROTTLE_SECONDS * 1000) { - return cachedAdvancedCheckResult.isDegraded; - } - } finally { - advancedCheckLock.writeLock().unlock(); - } - - AdvancedCheckResult result; - try (Connection conn = dataSource.getConnection()) { - result = performAdvancedMySQLChecks(conn); - } catch (Exception ex) { - if (ex.getCause() instanceof InterruptedException) { - Thread.currentThread().interrupt(); - } - logger.debug("Could not acquire connection for advanced checks: {}", ex.getMessage()); - result = new AdvancedCheckResult(false); - } - - advancedCheckLock.writeLock().lock(); - try { - lastAdvancedCheckTime = currentTime; - cachedAdvancedCheckResult = result; - return result.isDegraded; - } finally { - advancedCheckLock.writeLock().unlock(); + // Only one thread runs the expensive checks; others return stale cache + if (!advancedCheckInProgress.compareAndSet(false, true)) { + return cachedAdvancedCheckResult != null && cachedAdvancedCheckResult.isDegraded; + } + try { + AdvancedCheckResult result; + try (Connection conn = dataSource.getConnection()) { + result = performAdvancedMySQLChecks(conn); + } catch (Exception ex) { + if (ex.getCause() instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + logger.debug("Could not acquire connection for advanced checks: {}", ex.getMessage()); + result = new AdvancedCheckResult(false); + } + advancedCheckLock.writeLock().lock(); + try { + lastAdvancedCheckTime = currentTime; + cachedAdvancedCheckResult = result; + } finally { + advancedCheckLock.writeLock().unlock(); + } + return result.isDegraded; + } finally { + advancedCheckInProgress.set(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 359 - 391, The throttle window still allows a thundering herd because multiple threads can pass the stale double-check after advancedCheckLock.writeLock() is released; introduce an AtomicBoolean (e.g., advancedCheckInProgress) and use compareAndSet(true) to make exactly one thread perform the DB I/O in performAdvancedMySQLChecks via dataSource.getConnection(), while other threads immediately return the current cachedAdvancedCheckResult.isDegraded (or reload the cache if null) without performing queries; ensure the thread that won the compareAndSet clears advancedCheckInProgress in a finally block (and still updates lastAdvancedCheckTime and cachedAdvancedCheckResult under advancedCheckLock.writeLock()), and preserve existing InterruptedException handling and logging.
517-535: Consider using Javarecordtypes for these inner value classes.Since the project targets Java 17, these private inner classes are good candidates for records, reducing boilerplate:
Example
- private static class AdvancedCheckResult { - final boolean isDegraded; - - AdvancedCheckResult(boolean isDegraded) { - this.isDegraded = isDegraded; - } - } - - private static class HealthCheckResult { - final boolean isHealthy; - final String error; - final boolean isDegraded; - - HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) { - this.isHealthy = isHealthy; - this.error = error; - this.isDegraded = isDegraded; - } - } + private record AdvancedCheckResult(boolean isDegraded) {} + + private record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {}Note: record field access uses
isHealthy()instead of.isHealthy, so update all field references accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 517 - 535, Replace the private inner classes AdvancedCheckResult and HealthCheckResult with Java records to remove boilerplate: change "private static class AdvancedCheckResult { final boolean isDegraded; ... }" to "private record AdvancedCheckResult(boolean isDegraded) {}" and "private static class HealthCheckResult { final boolean isHealthy; final String error; final boolean isDegraded; ... }" to "private record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {}", remove explicit constructors, and update all usage sites to call the generated accessor methods (use isDegraded() and isHealthy() and error() instead of field access) wherever AdvancedCheckResult and HealthCheckResult fields were read.
201-219: Health check holds two pool connections simultaneously.The
try-with-resourcesat line 202 keeps the first connection open whileperformAdvancedMySQLChecksWithThrottle()(line 213) acquires a second connection internally (line 373). Under pool pressure — exactly the condition this check aims to detect — this doubles the footprint per health check thread.Close the initial connection before running advanced checks:
Suggested restructure
private HealthCheckResult checkMySQLHealthSync() { + // Basic connectivity check + try (Connection connection = dataSource.getConnection(); + PreparedStatement stmt = connection.prepareStatement("SELECT 1 as health_check")) { + stmt.setQueryTimeout((int) MYSQL_TIMEOUT_SECONDS); + try (ResultSet rs = stmt.executeQuery()) { + if (!rs.next()) { + return new HealthCheckResult(false, "No result from health check query", false); + } + } + } catch (Exception e) { + logger.warn("MySQL health check failed: {}", e.getMessage(), e); + return new HealthCheckResult(false, "MySQL connection failed", false); + } + + // Advanced checks use their own connection (acquired inside) try { - try (Connection connection = dataSource.getConnection(); - PreparedStatement stmt = connection.prepareStatement("SELECT 1 as health_check")) { - stmt.setQueryTimeout((int) MYSQL_TIMEOUT_SECONDS); - try (ResultSet rs = stmt.executeQuery()) { - if (!rs.next()) { - return new HealthCheckResult(false, "No result from health check query", false); - } - } - boolean isDegraded = performAdvancedMySQLChecksWithThrottle(); - return new HealthCheckResult(true, null, isDegraded); - } + boolean isDegraded = performAdvancedMySQLChecksWithThrottle(); + return new HealthCheckResult(true, null, isDegraded); } catch (Exception e) { - logger.warn("MySQL health check failed: {}", e.getMessage(), e); - return new HealthCheckResult(false, "MySQL connection failed", false); + logger.warn("Advanced MySQL checks failed: {}", e.getMessage(), e); + return new HealthCheckResult(true, null, false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java` around lines 201 - 219, checkMySQLHealthSync currently holds a Connection/PreparedStatement/ResultSet open while calling performAdvancedMySQLChecksWithThrottle, causing two pooled connections to be used; refactor so the simple "SELECT 1" query runs inside its own try-with-resources and is fully closed (close ResultSet, PreparedStatement and Connection) before invoking performAdvancedMySQLChecksWithThrottle; specifically update checkMySQLHealthSync to release the initial Connection/PreparedStatement/ResultSet after verifying rs.next() and only then call performAdvancedMySQLChecksWithThrottle, preserving existing return behavior and exception handling.pom.xml (1)
321-324: Duplicatespring-boot-maven-plugindeclarations.
spring-boot-maven-pluginis declared twice — once at lines 321–324 (no version, no execution config) and again at lines 422–433 (version 3.2.2,repackagegoal). Having two declarations for the same plugin can cause unexpected merge behavior in Maven's effective POM. Consider consolidating into a single block.Suggested consolidation
- <plugin> - <groupId>org.springframework.boot</groupId> - <artifactId>spring-boot-maven-plugin</artifactId> - </plugin>Keep only the explicit declaration at lines 422–433 that specifies the version and the
repackagegoal.Also applies to: 422-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 321 - 324, There are two declarations of the same Maven plugin (org.springframework.boot:spring-boot-maven-plugin); remove the duplicate by consolidating into a single plugin block: keep the explicit block that includes the version (3.2.2) and the repackage goal (the block referenced by the review at lines 422–433) and delete the redundant bare plugin block (the one with groupId/org.springframework.boot and artifactId/spring-boot-maven-plugin without version or executions), or merge any missing configuration from the bare block into the explicit block so only one org.springframework.boot:spring-boot-maven-plugin entry remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pom.xml`:
- Around line 390-409: The git-commit-id-maven-plugin configuration is now
correct: ensure <generateGitPropertiesFile> is set to true (as seen in the
git-commit-id-maven-plugin configuration) and keep
<failOnNoGitDirectory>false</failOnNoGitDirectory> and
<abbrevLength>7</abbrevLength> so VersionController can consume the generated
git.properties; no further code changes required.
In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java`:
- Around line 147-168: performHealthChecks previously risked leaving orphaned
tasks; the current change adds cancelFutures(mysqlFuture, redisFuture) in all
exception blocks—ensure the cancellation logic in cancelFutures safely handles
null and already-completed/ cancelled Future instances and that
awaitHealthChecks(mysqlFuture, redisFuture) can propagate
TimeoutException/InterruptedException appropriately; locate performHealthChecks,
awaitHealthChecks, cancelFutures, executorService.submit and the passed
checkMySQLHealthSync/checkRedisHealthSync methods to verify cancelFutures checks
for null, calls future.cancel(true) and logs outcomes, and that no other code
path can leave running tasks.
- Around line 420-457: The try/catch blocks in hasLockWaits and hasSlowQueries
swallow exceptions without including the exception details; update the catch
handlers to log the caught Exception (e) with the existing logger.debug call
(e.g., logger.debug("Could not check for lock waits", e) and logger.debug("Could
not check for slow queries", e)) so failures in those methods are recorded and
diagnosable while preserving the current behavior and timeouts.
---
Nitpick comments:
In `@pom.xml`:
- Around line 321-324: There are two declarations of the same Maven plugin
(org.springframework.boot:spring-boot-maven-plugin); remove the duplicate by
consolidating into a single plugin block: keep the explicit block that includes
the version (3.2.2) and the repackage goal (the block referenced by the review
at lines 422–433) and delete the redundant bare plugin block (the one with
groupId/org.springframework.boot and artifactId/spring-boot-maven-plugin without
version or executions), or merge any missing configuration from the bare block
into the explicit block so only one
org.springframework.boot:spring-boot-maven-plugin entry remains.
In `@src/main/java/com/iemr/helpline104/service/health/HealthService.java`:
- Around line 99-104: The constructor in HealthService is creating an
executorService with a hardcoded size of 6 (Executors.newFixedThreadPool(6))
even though only two concurrent tasks are used; change this to a pool size of 2
or make it configurable. Update the HealthService constructor to initialize
executorService with Executors.newFixedThreadPool(2) or fetch the size from a
configuration/constant (e.g., THREAD_POOL_SIZE) so the pool is small by default
but can be increased later without code changes; reference the HealthService
constructor and executorService/Executors.newFixedThreadPool(...) when making
the change.
- Around line 359-391: The throttle window still allows a thundering herd
because multiple threads can pass the stale double-check after
advancedCheckLock.writeLock() is released; introduce an AtomicBoolean (e.g.,
advancedCheckInProgress) and use compareAndSet(true) to make exactly one thread
perform the DB I/O in performAdvancedMySQLChecks via dataSource.getConnection(),
while other threads immediately return the current
cachedAdvancedCheckResult.isDegraded (or reload the cache if null) without
performing queries; ensure the thread that won the compareAndSet clears
advancedCheckInProgress in a finally block (and still updates
lastAdvancedCheckTime and cachedAdvancedCheckResult under
advancedCheckLock.writeLock()), and preserve existing InterruptedException
handling and logging.
- Around line 517-535: Replace the private inner classes AdvancedCheckResult and
HealthCheckResult with Java records to remove boilerplate: change "private
static class AdvancedCheckResult { final boolean isDegraded; ... }" to "private
record AdvancedCheckResult(boolean isDegraded) {}" and "private static class
HealthCheckResult { final boolean isHealthy; final String error; final boolean
isDegraded; ... }" to "private record HealthCheckResult(boolean isHealthy,
String error, boolean isDegraded) {}", remove explicit constructors, and update
all usage sites to call the generated accessor methods (use isDegraded() and
isHealthy() and error() instead of field access) wherever AdvancedCheckResult
and HealthCheckResult fields were read.
- Around line 201-219: checkMySQLHealthSync currently holds a
Connection/PreparedStatement/ResultSet open while calling
performAdvancedMySQLChecksWithThrottle, causing two pooled connections to be
used; refactor so the simple "SELECT 1" query runs inside its own
try-with-resources and is fully closed (close ResultSet, PreparedStatement and
Connection) before invoking performAdvancedMySQLChecksWithThrottle; specifically
update checkMySQLHealthSync to release the initial
Connection/PreparedStatement/ResultSet after verifying rs.next() and only then
call performAdvancedMySQLChecksWithThrottle, preserving existing return behavior
and exception handling.
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 Helpline104-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Improvements