[GitHub] [incubator-doris] morningman commented on a change in pull request #7155: [Config] Support disable query and load for backend to make Doris more robust and set default value to 1 for max_query_retry_time

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org