dataroaring commented on PR #59327:
URL: https://github.com/apache/doris/pull/59327#issuecomment-3716663978
## Code Review: Senior Database Engineer Perspective
### Overall Verdict: **LGTM with minor fixes** ✅
---
### Summary
Good memory optimization PR. The design cleanly separates cooldown-related
fields into `LocalTablet` to avoid unnecessary memory overhead in
`CloudTablet`. Saves ~272 bytes per tablet (~272 MB for 1M tablets).
---
### Issues Found
#### Must Fix Before Merge
**1. Misplaced comment in `LocalTablet.getRemoteDataSize()`**
```java
// Line 66-67
// if CooldownReplicaId is not init
// [fix](fe) move some variables from Tablet to LocalTablet which are not
used in CloudTablet
```
The second line appears to be a commit message accidentally left in the
code. Should be removed.
**2. Missing `@Override` annotation**
`LocalTablet.getRemoteDataSize()` overrides the parent method but lacks
`@Override` annotation.
#### Should Consider
**3. Inconsistent exception vs default value pattern**
```java
// Tablet.java
public void setCooldownConf(...) {
throw new UnsupportedOperationException("not support setCooldownConf in
Tablet");
}
public long getCooldownReplicaId() {
return -1; // Returns default instead of throwing
}
```
Setters throw exceptions but getters return defaults. This asymmetry could
be confusing. Consider either:
- Making `setCooldownConf` a no-op for consistency, OR
- Documenting the design decision clearly
**4. Test coverage**
The PR checklist shows no test option checked. While "no logic changed" is
claimed, there ARE behavioral changes:
- `Tablet.setCooldownConf()` now throws instead of silently updating fields
- `Tablet.getRemoteDataSize()` now always returns 0
Consider adding explicit unit tests for the refactored behavior.
---
### Positive Aspects ✅
1. **Clean inheritance design** - Clear separation of concerns between local
and cloud modes
2. **Proper Gson compatibility** -
`registerCompatibleSubtype(LocalTablet.class, Tablet.class.getSimpleName())`
handles deserialization of old data
3. **Good use of `@SerializedName` with alternates** - Maintains JSON
compatibility
4. **Comprehensive test updates** - All test files appropriately updated
5. **Significant memory savings** - 272B × 1M tablets = 272MB
---
### Recommendation
Approve after addressing the misplaced comment and adding the missing
`@Override` annotation. The design is sound and the optimization is valuable
for cloud deployments.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
