codelipenghui commented on PR #24471:
URL: https://github.com/apache/pulsar/pull/24471#issuecomment-3015656366
## Code Review
Thanks for implementing PIP-431! This is a well-designed feature that
addresses a real operational need. Overall the implementation looks solid with
good performance considerations. Here are some observations and suggestions:
### โ
Strengths
- Clean API design with proper backward compatibility using default methods
- Performance-conscious hybrid approach for lastPublishTimeStamp (in-memory
+ async recovery)
- Comprehensive test coverage including partitioned topics and edge cases
- Proper resource management with entry.release() in callbacks
### ๐ง Suggestions for Improvement
**1. Simplify async logic complexity in asyncGetStats()**
The current logic around lines 2748-2790 is quite complex. Consider this
simplification:
```java
private CompletableFuture<Long> getLastPublishTimestamp() {
long ledgerTime = ledger.getLastAddEntryTime();
return ledgerTime > 0
? CompletableFuture.completedFuture(ledgerTime)
: getLastMessagePublishTime();
}
```
**2. Consider defensive programming**
```java
public long getTopicCreationTimeStamp() {
return ledger != null ? ledger.getMetadataCreationTimestamp() : 0;
}
```
**3. Minor: CompletableFuture composition**
The nested thenCompose calls could be flattened for better readability,
though the current approach works correctly.
### ๐ Performance Analysis
- โ
Minimal impact on publish path (atomic long update)
- โ
Smart fallback mechanism avoids unnecessary I/O
- โ
One-time metadata read cost is acceptable
### ๐งช Testing
The test coverage is excellent - covers both regular and partitioned topics,
unloading, and truncation scenarios.
**Overall Assessment: 8/10** - Ready to merge with the suggested
simplifications if you agree with them. Great work on this feature!
--
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: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
