RockteMQ-AI commented on PR #4205:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4205#issuecomment-5606157209
## Code Review: PR #4205
**Summary:** Fix message-id query returning no rows when broker registers by
hostname instead of IP address. The solution adds DNS resolution to populate
both hostname:port and ip:port forms in the known broker endpoints set.
---
### ✅ Strengths
1. **Root cause analysis is solid** — The PR correctly identifies that
hostname-registered brokers cause IP mismatch in the topology guard
2. **Minimal, focused change** — Only modifies the endpoint registration
logic without touching the comparison logic
3. **IPv6 handling** — Properly strips brackets from IPv6 literals like
`[::1]:10911`
4. **Defensive coding** — Null checks, exception handling, and bounds
validation in `resolveBrokerAddrToIp()`
5. **Good test coverage** — Two clear test cases covering hostname and IP
scenarios with descriptive names
---
### ⚠️ Considerations
**1. DNS Resolution Performance** (Medium Priority)
```java
String ip = InetAddress.getByName(host).getHostAddress();
```
- `InetAddress.getByName()` performs synchronous DNS lookup
- Called during `knownBrokerEndpoints()` which runs on message query path
- **Impact:** Could add latency if DNS is slow or if there are many brokers
- **Suggestion:** Consider caching resolved IPs with a TTL (e.g., 5 minutes)
if this becomes a bottleneck
**2. Error Handling** (Low Priority)
```java
catch (Exception e) {
return null;
}
```
- Silent exception swallowing makes debugging harder
- **Suggestion:** Add debug logging: `log.debug("Failed to resolve broker
address: {}", brokerAddr, e);`
**3. Edge Case: Unresolvable Hostname** (Low Priority)
- If a broker registers with a hostname that cannot be resolved, the method
returns null and only the hostname form is added
- This is correct behavior (graceful degradation), but worth documenting
---
### 🔍 Code Quality
- **Correctness:** ✅ Logic is sound, handles both hostname and IP cases
- **Performance:** ⚠️ DNS resolution could add latency (acceptable for now)
- **Tests:** ✅ Good coverage with clear scenarios
- **Compatibility:** ✅ No breaking changes, backward compatible
---
### 💡 Suggestions
1. **Optional:** Add a comment explaining why we add both forms:
```java
// Add both hostname:port and ip:port forms to handle brokers registered
by either
```
2. **Optional:** Consider extracting the port parsing logic if reused
elsewhere:
```java
private static String extractPort(String addr, int sepIndex) {
return addr.substring(sepIndex + 1);
}
```
---
### 📊 Verdict
**APPROVE** — This is a well-implemented bug fix with solid test coverage.
The DNS resolution concern is valid but acceptable given the bounded number of
brokers and the critical nature of the bug. The graceful degradation (returning
null on resolution failure) ensures robustness.
Minor suggestions for logging and documentation are optional improvements.
---
*Review by @$SELF_LOGIN • Automated code analysis*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
