morningman commented on a change in pull request #7155:
URL: https://github.com/apache/incubator-doris/pull/7155#discussion_r755805485



##########
File path: 
fe/fe-core/src/main/java/org/apache/doris/analysis/ModifyBackendClause.java
##########
@@ -17,6 +17,7 @@
 
 package org.apache.doris.analysis;
 
+import org.apache.commons.lang3.StringUtils;

Review comment:
       import order

##########
File path: 
fe/fe-core/src/main/java/org/apache/doris/system/SystemInfoService.java
##########
@@ -759,8 +775,10 @@ public void releaseBackends(String clusterName, boolean 
isReplay) {
         return chosenBackendIds;
     }
 
-    public List<Long> seqChooseBackendIdsByStorageMediumAndTag(int backendNum, 
boolean needAlive, boolean isCreate,
-                                                               String 
clusterName, TStorageMedium storageMedium, Tag tag) {
+    public List<Long> seqChooseBackendIdsByStorageMediumAndTag(int backendNum, 
boolean scheduleAvailable,

Review comment:
       Too much `xxxAvailable` parameter. How about merge them into a struct, 
maybe like `predicates`?

##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/MetadataViewer.java
##########
@@ -84,7 +84,7 @@
                             
                             ReplicaStatus status = ReplicaStatus.OK;
                             Backend be = 
infoService.getBackend(replica.getBackendId());
-                            if (be == null || !be.isAvailable() || 
replica.isBad()) {
+                            if (be == null || !be.isAlive() || 
replica.isBad()) {

Review comment:
       After your modification, Even if BE is decommission, the replica status 
is OK.
   But when I execute `admin show replica status` stmt, I want to see all 
`abnormal` status.
   
   So I think better to make this check strictly.

##########
File path: 
fe/fe-core/src/main/java/org/apache/doris/clone/BackendLoadStatistic.java
##########
@@ -175,7 +175,7 @@ public void init() throws LoadBalanceException {
             throw new LoadBalanceException("backend " + beId + " does not 
exist");
         }
 
-        isAvailable = be.isAvailable();
+        isAvailable = be.isLoadAvailable();

Review comment:
       In BackendLoadStatistic, the `isAvailable` is used to determine whether 
this BE can be the destination of clone task.
   I think if BE is decommission, it should not accept any new clone task.
   So maybe this should be `isAvailable = be.isLoadAvailable() && 
be.isScheduleAvailable()` ?




-- 
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]

Reply via email to