Claude Code Review

PR #878 Review: Feature Negotiation for Rolling Upgrades

Overall Assessment: ✅ Approve

This is a well-designed and implemented feature that enables safe rolling upgrades through feature negotiation. The code is clean, well-structured, and includes comprehensive test coverage.

Strengths

1. Clean Architecture: The feature negotiation flow is well-organized:
   - feature.SupportedFeatures() - central registry of supported features
   - DataServerController.SupportedFeatures() - fetches per-node features via GetInfo RPC
   - negotiate() - computes intersection of features across quorum members
   - LeaderController.IsFeatureEnabled() - exposes negotiated features for use
2. Backward Compatibility: Old dataservers returning codes.Unimplemented are handled gracefully in syncDataServerInfo():
   if code == codes.Unimplemented {
   n.Warn("the storage data server is too old without info endpoint.")
   return nil // Treat as success with no features
   }
3. Robust Retry Logic: Uses exponential backoff for syncDataServerInfo() with proper error handling.
4. Good Test Coverage: Comprehensive tests for:
   - negotiate() function with edge cases (empty input, partial support, duplicates)
   - Rolling upgrade scenarios
   - IsFeatureEnabled() behavior
   - Integration tests with mocked different-version nodes

Code Quality

• Proto changes are clean and well-documented
• Proper use of atomic.Value for thread-safe feature storage
• Consistent error handling patterns
• Good logging with structured fields

Summary

The implementation is solid and ready to merge. The feature negotiation mechanism provides a clean foundation for introducing new capabilities while maintaining backward compatibility during rolling upgrades.