Copilot commented on code in PR #59460: URL: https://github.com/apache/doris/pull/59460#discussion_r2650677434
########## fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTablet.java: ########## @@ -0,0 +1,77 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.catalog; + +import org.apache.doris.common.Pair; + +import com.google.gson.annotations.SerializedName; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.util.Comparator; + +public class LocalTablet extends Tablet { + private static final Logger LOG = LogManager.getLogger(LocalTablet.class); + + // cooldown conf + @SerializedName(value = "cri", alternate = {"cooldownReplicaId"}) + private long cooldownReplicaId = -1; + @SerializedName(value = "ctm", alternate = {"cooldownTerm"}) + private long cooldownTerm = -1; + private final Object cooldownConfLock = new Object(); + + public LocalTablet() { + } + + public LocalTablet(long tabletId) { + super(tabletId); + } + + public void setCooldownConf(long cooldownReplicaId, long cooldownTerm) { + synchronized (cooldownConfLock) { + this.cooldownReplicaId = cooldownReplicaId; + this.cooldownTerm = cooldownTerm; + } + } + + public long getCooldownReplicaId() { + return cooldownReplicaId; + } + + public Pair<Long, Long> getCooldownConf() { + synchronized (cooldownConfLock) { + return Pair.of(cooldownReplicaId, cooldownTerm); + } + } + + @Override + public long getRemoteDataSize() { + // if CooldownReplicaId is not init + // [fix](fe) move some variables from Tablet to LocalTablet which are not used in CloudTablet + if (cooldownReplicaId <= 0) { + return 0; + } + for (Replica r : replicas) { + if (r.getId() == cooldownReplicaId) { + return r.getRemoteDataSize(); + } + } + // return replica with max remoteDataSize + return replicas.stream().max(Comparator.comparing(Replica::getRemoteDataSize)).get().getRemoteDataSize(); Review Comment: Potential NullPointerException when replicas list is empty. The method calls .get() on an Optional without checking if it's present first. If the replicas list is empty, stream().max() will return an empty Optional, and calling .get() will throw a NoSuchElementException. Consider using .orElse(0L) or checking if replicas is non-empty before this operation. ```suggestion return replicas.stream().mapToLong(Replica::getRemoteDataSize).max().orElse(0L); ``` ########## fe/fe-core/src/main/java/org/apache/doris/master/ReportHandler.java: ########## @@ -1533,7 +1535,7 @@ private static boolean addReplica(long tabletId, TabletMeta tabletMeta, TTabletI if (backendTabletInfo.isSetCooldownMetaId()) { // replica has cooldowned data do { - Pair<Long, Long> cooldownConf = tablet.getCooldownConf(); + Pair<Long, Long> cooldownConf = ((LocalTablet) tablet).getCooldownConf(); Review Comment: Unsafe cast to LocalTablet without type checking. The code casts tablet directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. ########## fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java: ########## @@ -394,7 +395,7 @@ public TConfirmUnusedRemoteFilesResult confirmUnusedRemoteFiles(TConfirmUnusedRe return; } // check cooldownReplicaId Review Comment: Unsafe cast to LocalTablet without type checking. The code casts tablet directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. ```suggestion // check cooldownReplicaId if (!(tablet instanceof LocalTablet)) { LOG.warn("tablet {} is not LocalTablet, skip confirming unused remote files", info.tablet_id); return; } ``` ########## fe/fe-core/src/main/java/org/apache/doris/cooldown/CooldownConfHandler.java: ########## @@ -135,7 +136,7 @@ private static Tablet getTablet(CooldownConf conf) { public static void replayUpdateCooldownConf(CooldownConfList cooldownConfList) { cooldownConfList.getCooldownConf().forEach(conf -> { - Tablet tablet = getTablet(conf); + LocalTablet tablet = (LocalTablet) getTablet(conf); if (tablet != null) { tablet.setCooldownConf(conf.cooldownReplicaId, conf.cooldownTerm); Review Comment: Unsafe cast to LocalTablet without type checking. The code casts the result of getTablet() directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. ```suggestion Tablet tablet = getTablet(conf); if (tablet instanceof LocalTablet) { LocalTablet localTablet = (LocalTablet) tablet; localTablet.setCooldownConf(conf.cooldownReplicaId, conf.cooldownTerm); } else if (tablet != null) { LOG.warn("skip setting cooldown conf for non-local tablet. tabletId={}", conf.tabletId); ``` ########## fe/fe-core/src/main/java/org/apache/doris/catalog/LocalTablet.java: ########## @@ -0,0 +1,77 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.catalog; + +import org.apache.doris.common.Pair; + +import com.google.gson.annotations.SerializedName; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.util.Comparator; + +public class LocalTablet extends Tablet { + private static final Logger LOG = LogManager.getLogger(LocalTablet.class); + + // cooldown conf + @SerializedName(value = "cri", alternate = {"cooldownReplicaId"}) + private long cooldownReplicaId = -1; + @SerializedName(value = "ctm", alternate = {"cooldownTerm"}) + private long cooldownTerm = -1; + private final Object cooldownConfLock = new Object(); + + public LocalTablet() { + } + + public LocalTablet(long tabletId) { + super(tabletId); + } + + public void setCooldownConf(long cooldownReplicaId, long cooldownTerm) { + synchronized (cooldownConfLock) { + this.cooldownReplicaId = cooldownReplicaId; + this.cooldownTerm = cooldownTerm; + } + } + + public long getCooldownReplicaId() { + return cooldownReplicaId; + } + + public Pair<Long, Long> getCooldownConf() { + synchronized (cooldownConfLock) { + return Pair.of(cooldownReplicaId, cooldownTerm); + } + } + + @Override + public long getRemoteDataSize() { + // if CooldownReplicaId is not init + // [fix](fe) move some variables from Tablet to LocalTablet which are not used in CloudTablet Review Comment: Commit message left in code comment. The comment on line 65 appears to be a commit message that was accidentally included in the code. This should be removed or rewritten as a proper code comment explaining the logic. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/LocalReplica.java: ########## @@ -0,0 +1,97 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.catalog; + +import org.apache.doris.thrift.TUniqueId; + +import com.google.gson.annotations.SerializedName; +import lombok.Getter; +import lombok.Setter; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +public class LocalReplica extends Replica { + private static final Logger LOG = LogManager.getLogger(LocalReplica.class); + + @SerializedName(value = "rds", alternate = {"remoteDataSize"}) + private volatile long remoteDataSize = 0; + @Setter + @Getter + @SerializedName(value = "ris", alternate = {"remoteInvertedIndexSize"}) + private Long remoteInvertedIndexSize = 0L; + @Setter + @Getter + @SerializedName(value = "rss", alternate = {"remoteSegmentSize"}) + private Long remoteSegmentSize = 0L; + + private TUniqueId cooldownMetaId; + private long cooldownTerm = -1; + + public LocalReplica() { + } + + public LocalReplica(ReplicaContext context) { + super(context); + } + + // for rollup + // the new replica's version is -1 and last failed version is -1 + public LocalReplica(long replicaId, long backendId, int schemaHash, ReplicaState state) { + super(replicaId, backendId, schemaHash, state); + } + + // for create tablet and restore + public LocalReplica(long replicaId, long backendId, ReplicaState state, long version, int schemaHash) { + super(replicaId, backendId, state, version, schemaHash); + } + + public LocalReplica(long replicaId, long backendId, long version, int schemaHash, long dataSize, + long remoteDataSize, long rowCount, ReplicaState state, long lastFailedVersion, long lastSuccessVersion) { + super(replicaId, backendId, version, schemaHash, dataSize, remoteDataSize, rowCount, state, lastFailedVersion, + lastSuccessVersion); + this.remoteDataSize = remoteDataSize; + } + + public long getRemoteDataSize() { + return remoteDataSize; + } + + public void setRemoteDataSize(long remoteDataSize) { + this.remoteDataSize = remoteDataSize; + } Review Comment: Missing @Override annotation for methods that override parent class methods. The methods getRemoteInvertedIndexSize, setRemoteInvertedIndexSize, getRemoteSegmentSize, and setRemoteSegmentSize override base class methods but lack the @Override annotation, which should be added for clarity and to prevent errors if the parent method signature changes. ########## fe/fe-core/src/main/java/org/apache/doris/cooldown/CooldownConfHandler.java: ########## @@ -100,7 +101,7 @@ private void updateCooldownConf(List<CooldownConf> confToUpdate) { // update Tablet for (CooldownConf conf : updatedConf) { - Tablet tablet = tabletMap.get(conf.tabletId); + LocalTablet tablet = (LocalTablet) tabletMap.get(conf.tabletId); Review Comment: Unsafe cast to LocalTablet without type checking. The code casts the result from tabletMap.get() directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.java: ########## @@ -289,11 +275,33 @@ public void setDataSize(long dataSize) { } public long getRemoteDataSize() { - return remoteDataSize; + return 0; } public void setRemoteDataSize(long remoteDataSize) { - this.remoteDataSize = remoteDataSize; + if (remoteDataSize > 0) { + throw new UnsupportedOperationException("setRemoteDataSize is not supported in Replica"); + } + } + + public Long getRemoteInvertedIndexSize() { + return 0L; + } + + public void setRemoteInvertedIndexSize(long remoteInvertedIndexSize) { + if (remoteInvertedIndexSize > 0) { + throw new UnsupportedOperationException("setRemoteInvertedIndexSize is not supported in Replica"); + } + } + + public Long getRemoteSegmentSize() { + return 0L; + } + + public void setRemoteSegmentSize(long remoteSegmentSize) { + if (remoteSegmentSize > 0) { + throw new UnsupportedOperationException("setRemoteSegmentSize is not supported in Replica"); + } } Review Comment: Throwing UnsupportedOperationException in setter may break existing code paths. Similar to setRemoteDataSize, these methods throw UnsupportedOperationException but may be called from existing code paths. Verify that all call sites properly handle this exception or ensure these methods are never called on base Replica instances. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/TabletInvertedIndex.java: ########## @@ -672,14 +672,14 @@ private boolean needSync(Replica replicaInFe, TTabletInfo backendTabletInfo) { private void handleCooldownConf(TabletMeta tabletMeta, TTabletInfo beTabletInfo, List<CooldownConf> cooldownConfToPush, List<CooldownConf> cooldownConfToUpdate) { - Tablet tablet; + LocalTablet tablet; try { OlapTable table = (OlapTable) Env.getCurrentInternalCatalog().getDbNullable(tabletMeta.getDbId()) .getTable(tabletMeta.getTableId()) .get(); table.readLock(); try { - tablet = table.getPartition(tabletMeta.getPartitionId()).getIndex(tabletMeta.getIndexId()) + tablet = (LocalTablet) table.getPartition(tabletMeta.getPartitionId()).getIndex(tabletMeta.getIndexId()) .getTablet(beTabletInfo.tablet_id); Review Comment: Unsafe cast to LocalTablet without type checking. The code casts the result of getTablet() directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. ```suggestion Tablet baseTablet = table.getPartition(tabletMeta.getPartitionId()) .getIndex(tabletMeta.getIndexId()) .getTablet(beTabletInfo.tablet_id); if (!(baseTablet instanceof LocalTablet)) { LOG.warn("tablet {} is not LocalTablet, skip handling cooldown conf", beTabletInfo.tablet_id); return; } tablet = (LocalTablet) baseTablet; ``` ########## fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.java: ########## @@ -289,11 +275,33 @@ public void setDataSize(long dataSize) { } public long getRemoteDataSize() { - return remoteDataSize; + return 0; } public void setRemoteDataSize(long remoteDataSize) { - this.remoteDataSize = remoteDataSize; + if (remoteDataSize > 0) { + throw new UnsupportedOperationException("setRemoteDataSize is not supported in Replica"); + } + } Review Comment: Throwing UnsupportedOperationException in setter may break existing code paths. The setRemoteDataSize method now throws UnsupportedOperationException when value > 0, but the base class Replica has an updateWithReport method (line 414) that calls this setter. If a CloudReplica receives a tablet report with remoteDataSize > 0, this will throw an exception and break the report handling. Consider either overriding updateWithReport in subclasses or checking the replica type before calling these setters. ########## fe/fe-core/src/main/java/org/apache/doris/cooldown/CooldownConfHandler.java: ########## @@ -83,7 +84,7 @@ private void updateCooldownConf(List<CooldownConf> confToUpdate) { int index = rand.nextInt(replicas.size()); conf.setCooldownReplicaId(replicas.get(index).getId()); // find TabletMeta to get cooldown term - Tablet tablet = getTablet(conf); + LocalTablet tablet = (LocalTablet) getTablet(conf); Review Comment: Unsafe cast to LocalTablet without type checking. The code casts the result of getTablet() directly to LocalTablet without verifying the type first. If running in cloud mode with CloudTablets, this will throw a ClassCastException. Consider adding an instanceof check before casting or verifying that this code path is never executed in cloud mode. -- 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]
