dataroaring commented on PR #59814:
URL: https://github.com/apache/doris/pull/59814#issuecomment-3750900102
## PR Review: `[fix](fe) modify replicas to replica in CloudTablet`
### Summary
This PR refactors the `Tablet` class hierarchy to properly differentiate
between cloud and local tablet implementations:
1. Converting `Tablet` to an abstract class
2. Moving replica management to subclasses (`LocalTablet` and `CloudTablet`)
3. Changing `CloudTablet` to use a single `Replica` field instead of
`List<Replica>` (since cloud mode only has one replica)
### Overall Assessment: ⚠️ **Needs Revision**
---
### Critical Issues
#### 1. Missing `hashCode()` implementation violates Java contract 🔴
Both `LocalTablet` and `CloudTablet` override `equals()` but do not override
`hashCode()`. This violates Java equals/hashCode contract and will cause bugs
if tablets are used in `HashMap`, `HashSet`, or any hash-based collections.
**LocalTablet.java** - needs:
```java
@Override
public int hashCode() {
return Objects.hash(id, replicas);
}
```
**CloudTablet.java** - needs:
```java
@Override
public int hashCode() {
return Objects.hash(id, replica);
}
```
#### 2. Potential breaking change:
`deleteReplica`/`deleteReplicaByBackendId` throw UnsupportedOperationException
for CloudTablet 🔴
The base `Tablet` class now throws `UnsupportedOperationException` for these
methods, and `CloudTablet` does not override them:
```java
// Tablet.java
public boolean deleteReplica(Replica replica) {
throw new UnsupportedOperationException("deleteReplica is not supported
in Tablet");
}
```
These methods are called from:
- `TabletSchedCtx.java:934` and `:968`
- `ReportHandler.java:1124`
- `InternalCatalog.java:1165`
**Please verify that these code paths are never executed in cloud mode, or
implement these methods in `CloudTablet`.**
#### 3. `readyToBeRepaired` also throws UnsupportedOperationException 🔴
Same issue - this method is called from `TabletScheduler`, `TabletChecker`,
and `ColocateTableCheckerAndBalancer`. If tablet repair is disabled in cloud
mode, this should be documented; otherwise, implement it.
---
### Medium Issues
#### 4. Redundant null check in LocalTablet constructor 🟡
```java
public LocalTablet(long tabletId) {
super(tabletId);
if (this.replicas == null) { // Always true for a new instance
this.replicas = new ArrayList<>();
}
...
}
```
Consider using a field initializer or implementing `GsonPostProcessable`
like `CloudTablet` does.
#### 5. CloudTablet.getReplicas() creates new ArrayList on every call 🟡
```java
@Override
public List<Replica> getReplicas() {
if (replica == null) {
return Lists.newArrayList();
}
return Lists.newArrayList(replica);
}
```
This allocates a new `ArrayList` on every call, which could be problematic
in hot paths. Consider:
```java
@Override
public List<Replica> getReplicas() {
if (replica == null) {
return Collections.emptyList();
}
return Collections.singletonList(replica);
}
```
#### 6. PR title/description does not match scope 🟡
The actual changes are much broader than "modify replicas to replica in
CloudTablet":
- Making `Tablet` abstract
- Moving 150+ lines of code to `LocalTablet`
- Changing method signatures and contracts
The description should be updated to accurately reflect the architectural
refactoring.
---
### Minor Issues
#### 7. `isLatestReplicaAndDeleteOld` could be simplified in CloudTablet 🟢
As @deardeng noted, since cloud only has one replica, this method could be
inlined:
```java
@Override
public void addReplica(Replica newReplica, boolean isRestore) {
if (replica == null || replica.getVersion() <= newReplica.getVersion()) {
this.replica = newReplica;
if (!isRestore) {
Env.getCurrentInvertedIndex().addReplica(id, newReplica);
}
}
}
```
---
### Test Coverage Concern
The test changes only remove `clearReplica()` test. Given the significant
refactoring:
- No new tests for `LocalTablet`
- No tests for `CloudTablet.gsonPostProcess()` migration
- No tests verifying `UnsupportedOperationException` behavior
---
### Recommendation
**Hold merge** until:
1. Add `hashCode()` implementations to both `LocalTablet` and `CloudTablet`
2. Verify or document that
`deleteReplica`/`deleteReplicaByBackendId`/`readyToBeRepaired` are never called
in cloud mode (or implement them)
3. Update PR description to reflect actual scope of changes
4. Consider the performance optimization for `CloudTablet.getReplicas()`
--
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]
