Copilot commented on code in PR #59814:
URL: https://github.com/apache/doris/pull/59814#discussion_r2685029804
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTablet.java:
##########
@@ -64,36 +75,68 @@ public Multimap<Long, Long>
getNormalReplicaBackendPathMap() throws UserExceptio
return backendPathMapReprocess(pathMap);
}
+ @Override
public Multimap<Long, Long> getNormalReplicaBackendPathMapCloud(String
beEndpoint) throws UserException {
Multimap<Long, Long> pathMap =
super.getNormalReplicaBackendPathMapCloud(beEndpoint);
return backendPathMapReprocess(pathMap);
}
- @Override
- protected boolean isLatestReplicaAndDeleteOld(Replica newReplica) {
+ private boolean isLatestReplicaAndDeleteOld(Replica newReplica) {
boolean delete = false;
boolean hasBackend = false;
- long version = newReplica.getVersion();
- Iterator<Replica> iterator = replicas.iterator();
- while (iterator.hasNext()) {
+ if (replica != null) {
hasBackend = true;
- Replica replica = iterator.next();
- if (replica.getVersion() <= version) {
- iterator.remove();
+ if (replica.getVersion() <= newReplica.getVersion()) {
+ replica = null;
delete = true;
}
}
return delete || !hasBackend;
}
+ @Override
public void addReplica(Replica replica, boolean isRestore) {
if (isLatestReplicaAndDeleteOld(replica)) {
- replicas.add(replica);
+ this.replica = replica;
if (!isRestore) {
Env.getCurrentInvertedIndex().addReplica(id, replica);
}
}
}
+ @Override
+ public List<Replica> getReplicas() {
+ if (replica == null) {
+ return Lists.newArrayList();
+ }
+ return Lists.newArrayList(replica);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof CloudTablet)) {
+ return false;
+ }
+ CloudTablet tablet = (CloudTablet) obj;
Review Comment:
The equals method can throw a NullPointerException when replica is null. The
method should check if both this.replica and tablet.replica are null before
calling equals. Consider adding null checks: if (replica == null) return
tablet.replica == null && id == tablet.id;
```suggestion
CloudTablet tablet = (CloudTablet) obj;
if (replica == null) {
return tablet.replica == null && id == tablet.id;
}
if (tablet.replica == null) {
return false;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTablet.java:
##########
@@ -161,4 +175,153 @@ public long getLastCheckTime() {
public void setLastCheckTime(long lastCheckTime) {
this.lastCheckTime = lastCheckTime;
}
+
+ private boolean isLatestReplicaAndDeleteOld(Replica newReplica) {
+ boolean delete = false;
+ boolean hasBackend = false;
+ long version = newReplica.getVersion();
+ Iterator<Replica> iterator = replicas.iterator();
+ while (iterator.hasNext()) {
+ Replica replica = iterator.next();
+ if (replica.getBackendIdWithoutException() ==
newReplica.getBackendIdWithoutException()) {
+ hasBackend = true;
+ if (replica.getVersion() <= version) {
+ iterator.remove();
+ delete = true;
+ }
+ }
+ }
+
+ return delete || !hasBackend;
+ }
+
+ @Override
+ public void addReplica(Replica replica, boolean isRestore) {
+ if (isLatestReplicaAndDeleteOld(replica)) {
+ replicas.add(replica);
+ if (!isRestore) {
+ Env.getCurrentInvertedIndex().addReplica(id, replica);
+ }
+ }
+ }
+
+ @Override
+ public List<Replica> getReplicas() {
+ return this.replicas;
+ }
+
+ @Override
+ public Replica getReplicaByBackendId(long backendId) {
+ for (Replica replica : replicas) {
+ if (replica.getBackendIdWithoutException() == backendId) {
+ return replica;
+ }
+ }
+ return null;
+ }
+
+ @Override
+ public boolean deleteReplica(Replica replica) {
+ if (replicas.contains(replica)) {
+ replicas.remove(replica);
+ Env.getCurrentInvertedIndex().deleteReplica(id,
replica.getBackendIdWithoutException());
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public boolean deleteReplicaByBackendId(long backendId) {
+ Iterator<Replica> iterator = replicas.iterator();
+ while (iterator.hasNext()) {
+ Replica replica = iterator.next();
+ if (replica.getBackendIdWithoutException() == backendId) {
+ iterator.remove();
+ Env.getCurrentInvertedIndex().deleteReplica(id, backendId);
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof LocalTablet)) {
+ return false;
+ }
+
+ LocalTablet tablet = (LocalTablet) obj;
+
+ if (replicas != tablet.replicas) {
Review Comment:
The LocalTablet equals method can throw NullPointerException if either
this.replicas or tablet.replicas is null. While unlikely in practice since the
constructor initializes replicas, deserialization could potentially create
instances without going through the constructor. Consider adding null checks
before accessing replicas fields.
```suggestion
if (replicas != tablet.replicas) {
if (replicas == null || tablet.replicas == null) {
return false;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTablet.java:
##########
@@ -63,6 +74,9 @@ public LocalTablet() {
public LocalTablet(long tabletId) {
super(tabletId);
+ if (this.replicas == null) {
+ this.replicas = new ArrayList<>();
+ }
Review Comment:
The LocalTablet constructor checks if replicas is null, but since this is a
new field declaration (not inherited), it will always be null at this point.
The check is correct but could be simplified by always initializing to new
ArrayList since the field will always be null when the constructor runs.
Consider simplifying to: this.replicas = new ArrayList<>();
```suggestion
this.replicas = new ArrayList<>();
```
--
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]
