[Improve] add connection check when get connection #10
Conversation
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
…luo-yx/hertzbeat into 0722-yuluo/optimize-collector
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache
participant Connection
Client->>Cache: request getConnection()
Cache->>Connection: check()
alt check() succeeds
Cache->>Client: return connection
else check() fails (throws)
Cache->>Client: return null (log error)
Cache->>Connection: closeConnection() (async, if applicable)
Cache->>Cache: remove cache entry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the robustness of connection management across various data sources by introducing a standardized mechanism to verify connection validity before use. It refactors existing connection classes to incorporate a new check() method, ensuring that stale or closed connections are identified and handled proactively, thereby improving the reliability of data collection.
Highlights
- Standardized Connection Health Check: Introduced an abstract
check()method inAbstractConnection.javato define a common interface for verifying the health of a connection before it is used. - Concrete Connection Implementations: Implemented the new
check()method in various concrete connection classes (JdbcConnect,JmxConnect,MongodbConnect,RedfishConnect,RedisConnect,SshConnect), each with specific logic tailored to its connection type (e.g.,isClosed(),pingcommand,isOpen()). - Proactive Connection Validation: Modified the
getConnection()method in all affected concrete connection classes to call the newcheck()method. If the check fails, an error is logged, andnullis returned, preventing the use of invalid or stale connections. - Refactored Redis Connection Handling: Updated
RedisCommonCollectImpl.javato leverage the newcheck()mechanism when retrieving Redis connections from the cache, improving the handling of invalid cached connections by attempting to close and remove them upon detection.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
🤖 Refacto.AI: PR Summary & Reviewer Guide🎯 1. The Goal (Why was this PR raised?)This PR aims to improve connection handling by adding connection validation checks when retrieving connections from cache. The implementation adds a new 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: Connection Handling Logic
Area of Concern: Connection Validation Implementation
Area of Concern: Error Handling in Connection Classes
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
There was a problem hiding this comment.
Code Review
This pull request introduces a connection check when retrieving a connection from the cache for various protocols (JDBC, JMX, MongoDB, Redfish, Redis, SSH). The goal is to ensure that clients receive a valid, open connection.
The overall approach is good, but there are several areas for improvement:
- A critical logic error in
RedisCommonCollectImplprevents invalid connections from being removed from the cache. - Exception logging is insufficient across all new connection wrapper classes, which will make debugging harder.
- The connection check for JMX is unreliable.
- There are also some minor code style and naming issues.
I've provided detailed comments and suggestions to address these points.
...ector/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JdbcConnect.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JmxConnect.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java (1)
1-62: Standardize exception handling incheck()methodsThere’s inconsistent use of checked vs. unchecked exceptions across your
AbstractConnectionsubclasses. To improve maintainability and clarity, please choose a single strategy (e.g. all methods throw a checkedExceptionor a customConnectionException, or make them all unchecked) and apply it uniformly.Files requiring updates:
- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/SshConnect.java
• currently throws and declares genericException—OK if you choose checked, otherwise adjust.- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedisConnect.java
• declaresthrows Exceptionbut usesthrow new RuntimeException(...).- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java
• same mismatch (throws Exceptionvs.RuntimeException).- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JmxConnect.java
• same mismatch.- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JdbcConnect.java
• narrows signature tothrows SQLException. Decide whether to align on generic Exception or a shared custom exception.- collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/MongodbConnect.java
• currently relies on driver’s unchecked exceptions—consider making its exception behavior explicit.Suggested paths forward:
- If you stick with the existing
throws Exceptionin the abstract base, change all subclasses tothrow new Exception("…")(or a custom checked exception) instead ofRuntimeException.- Alternatively, introduce a
ConnectionException extends Exception(orRuntimeException) and have everycheck()throw it, updating both signature and throw sites.- If you prefer unchecked errors, update the abstract method to remove
throws Exceptionand let subclasses consistently throwRuntimeExceptionor your custom unchecked type.Please update the above files to reflect your chosen approach.
🧹 Nitpick comments (5)
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/SshConnect.java (1)
49-59: Good error handling pattern with minor suggestion.The error handling approach is solid - calling
check()before returning the connection and gracefully handling failures by logging and returningnull. This prevents invalid connections from being used downstream.Consider being more specific about the exception type in the error message:
catch (Exception e) { - log.error(e.getMessage()); + log.error("SSH connection validation failed: {}", e.getMessage()); return null; }collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JdbcConnect.java (1)
54-60: Improve error logging to include stack trace.The current logging only captures the exception message, which may not provide sufficient debugging information. Consider logging the full exception.
try { this.check(); } catch (SQLException e) { - log.error(e.getMessage()); + log.error("JDBC connection check failed: {}", e.getMessage(), e); return null; }collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JmxConnect.java (1)
54-60: Improve error logging consistency.The error message format should be consistent with other connection classes and include more context.
try { this.check(); } catch (Exception e) { - log.error(e.getMessage()); + log.error("JMX connection check failed: {}", e.getMessage(), e); return null; }collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/MongodbConnect.java (1)
42-46: Consider alternative database for ping command.Using the "admin" database for the ping command may fail if the MongoDB user doesn't have access to the admin database. Consider using a more accessible approach.
Consider using the default database or a more permissive approach:
@Override public void check() throws Exception { - mongoClient.getDatabase("admin").runCommand(new Document("ping", 1)); + // Try to get database names as a connectivity check, which typically requires fewer permissions + mongoClient.listDatabaseNames().first(); }Alternatively, you could catch permission-related exceptions and try a fallback approach.
collector/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java (1)
248-248: Consider using structured logging for better observability.The current logging is informative but could benefit from structured logging to make it easier to monitor connection health issues.
-log.info("The Redis connection from cache is invalid, closing and removing: {}", e.getMessage()); +log.warn("Redis connection validation failed, cleaning up cached connection. Identifier: {}, Error: {}", + identifier, e.getMessage(), e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/AbstractConnection.java(2 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JdbcConnect.java(2 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JmxConnect.java(1 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/MongodbConnect.java(2 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java(1 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedisConnect.java(1 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/SshConnect.java(1 hunks)collector/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java(1 hunks)collector/src/test/java/org/apache/hertzbeat/collector/collect/common/cache/CommonCacheTest.java(1 hunks)
🔇 Additional comments (8)
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/AbstractConnection.java (2)
36-39: LGTM! Well-designed abstract contract for connection validation.The introduction of the abstract
check()method establishes a consistent contract for connection validation across all subclasses. The method signature appropriately throwsExceptionto allow flexibility in implementation-specific error handling.
44-44: Good improvement in method clarity.Making the call to
this.closeConnection()explicit improves code readability and makes the delegation pattern more obvious.collector/src/test/java/org/apache/hertzbeat/collector/collect/common/cache/CommonCacheTest.java (1)
49-51: LGTM! Appropriate test adaptation for the new abstract method.The empty implementation of
check()is suitable for the mock connection used in cache testing, as the focus is on testing cache behavior rather than connection validation logic.collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/SshConnect.java (1)
41-47: LGTM! Clear and appropriate connection validation.The implementation correctly checks if the SSH session is open and provides a descriptive error message. The validation logic is straightforward and appropriate for SSH connections.
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedisConnect.java (1)
50-61: LGTM! Consistent error handling pattern.The error handling approach matches the pattern established in other connection classes - validating before returning and gracefully handling failures with logging.
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java (1)
49-61: LGTM! Consistent error handling pattern.The implementation follows the established pattern of validating connections before returning them and handling failures gracefully with appropriate logging.
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/MongodbConnect.java (1)
51-57: LGTM - Consistent error handling implementation.The error handling pattern is consistent with other connection classes and properly logs errors before returning null.
collector/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java (1)
245-256: Excellent error handling and cleanup logic.The refactored connection retrieval properly handles invalid connections by:
- Catching RuntimeException from the new check() mechanism
- Attempting to close the connection asynchronously
- Removing the invalid connection from cache
- Providing informative logging
This prevents accumulation of invalid connections and ensures proper resource cleanup.
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JdbcConnect.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/JmxConnect.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedfishConnect.java
Show resolved
Hide resolved
collector/src/main/java/org/apache/hertzbeat/collector/collect/common/cache/RedisConnect.java
Show resolved
Hide resolved
Solid Foundation - Critical Connection Validation Improvements Needed!
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void check() throws SQLException { | ||
|
|
||
| if (connection.isClosed()) { | ||
| throw new SQLException("Connection is closed"); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Connection getConnection() { | ||
|
|
||
| try { | ||
| this.check(); | ||
| } | ||
| catch (SQLException e) { | ||
| log.error(e.getMessage()); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Returning null instead of throwing exceptions on connection validation failures
The getConnection() method silently returns null when connection validation fails instead of propagating the exception. This violates the fail-fast principle and will lead to NullPointerExceptions when client code attempts to use the connection. Since the method signature doesn't indicate that null can be returned, callers are unlikely to check for null values, resulting in runtime failures that are difficult to diagnose and potentially causing system instability.
| } | |
| } | |
| @Override | |
| public void check() throws SQLException { | |
| if (connection.isClosed()) { | |
| throw new SQLException("Connection is closed"); | |
| } | |
| } | |
| @Override | |
| public Connection getConnection() { | |
| try { | |
| this.check(); | |
| } | |
| catch (SQLException e) { | |
| log.error(e.getMessage()); | |
| return null; | |
| } | |
| @Override | |
| public Connection getConnection() { | |
| try { | |
| this.check(); | |
| return connection; | |
| } | |
| catch (SQLException e) { | |
| log.error("Connection validation failed", e); | |
| throw new RuntimeException("Failed to get valid JDBC connection", e); | |
| } | |
| } |
| @Override | ||
| public void check() throws Exception { | ||
|
|
||
| if (connection.getConnectionId().isEmpty()) { | ||
| throw new RuntimeException("connection is closed"); |
There was a problem hiding this comment.
Potential NullPointerException in JmxConnect.check()
The JmxConnect.check() method doesn't handle the case where the connection itself might be null. If the connection object is null (which could happen if connection initialization failed), calling connection.getConnectionId() will throw a NullPointerException. This would bypass the intended error handling in the getConnection() method.
| @Override | |
| public void check() throws Exception { | |
| if (connection.getConnectionId().isEmpty()) { | |
| throw new RuntimeException("connection is closed"); | |
| if (connection == null || connection.getConnectionId().isEmpty()) { | |
| throw new RuntimeException("connection is null or closed"); |
| * @return connection | ||
| */ | ||
| private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) { | ||
| StatefulConnection<String, String> connection = null; | ||
|
|
||
| Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true); | ||
|
|
||
| if (cacheOption.isPresent()) { | ||
| RedisConnect redisConnect = cacheOption.get(); | ||
| connection = redisConnect.getConnection(); | ||
| if (!connection.isOpen()) { | ||
|
|
||
| try { | ||
| return redisConnect.getConnection(); | ||
| } catch (RuntimeException e) { | ||
| log.info("The Redis connection from cache is invalid, closing and removing: {}", e.getMessage()); | ||
| try { | ||
| connection.closeAsync(); | ||
| } catch (Exception e) { | ||
| log.info("The redis connect form cache, close error: {}", e.getMessage()); | ||
| redisConnect.getConnection().closeAsync(); | ||
| } catch (Exception closeException) { | ||
| log.info("Error closing Redis connection: {}", closeException.getMessage()); | ||
| } | ||
| connection = null; | ||
| connectionCommonCache.removeCache(identifier); | ||
| } | ||
| } | ||
| return connection; | ||
|
|
||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Inefficient Redis Connection Management in RedisCommonCollectImpl
The getStatefulConnection method has been updated to use the new check() functionality, but it makes a second call to getConnection() when closing an invalid connection. This creates a performance inefficiency as it could trigger another validation check and potentially another exception. Additionally, it logs connection issues at INFO level instead of ERROR or WARN, which could hide important connection problems in production environments.
| * @return connection | |
| */ | |
| private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) { | |
| StatefulConnection<String, String> connection = null; | |
| Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true); | |
| if (cacheOption.isPresent()) { | |
| RedisConnect redisConnect = cacheOption.get(); | |
| connection = redisConnect.getConnection(); | |
| if (!connection.isOpen()) { | |
| try { | |
| return redisConnect.getConnection(); | |
| } catch (RuntimeException e) { | |
| log.info("The Redis connection from cache is invalid, closing and removing: {}", e.getMessage()); | |
| try { | |
| connection.closeAsync(); | |
| } catch (Exception e) { | |
| log.info("The redis connect form cache, close error: {}", e.getMessage()); | |
| redisConnect.getConnection().closeAsync(); | |
| } catch (Exception closeException) { | |
| log.info("Error closing Redis connection: {}", closeException.getMessage()); | |
| } | |
| connection = null; | |
| connectionCommonCache.removeCache(identifier); | |
| } | |
| } | |
| return connection; | |
| return null; | |
| } | |
| private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) { | |
| Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true); | |
| if (cacheOption.isPresent()) { | |
| RedisConnect redisConnect = cacheOption.get(); | |
| StatefulConnection<String, String> connection = null; | |
| try { | |
| connection = redisConnect.getConnection(); | |
| return connection; | |
| } catch (RuntimeException e) { | |
| log.warn("The Redis connection from cache is invalid, removing: {}", e.getMessage()); | |
| try { | |
| if (connection != null) { | |
| connection.closeAsync(); | |
| } | |
| } catch (Exception closeException) { | |
| log.warn("Error closing Redis connection: {}", closeException.getMessage()); | |
| } | |
| connectionCommonCache.removeCache(identifier); | |
| } | |
| } | |
| return null; |
What's changed?
Checklist
Add or update API
Summary by CodeRabbit
New Features
Bug Fixes
Tests