Re: [PR] [improve][broker][pip-431] PIP-431: Add Creation and Last Publish Timestamps to Topic Stats [pulsar]

["codelipenghui (via GitHub)" <gi...@apache.org>]

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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org