Copilot commented on code in PR #56601:
URL: https://github.com/apache/doris/pull/56601#discussion_r2492835449
##########
fe/fe-core/src/main/java/org/apache/doris/system/Backend.java:
##########
@@ -293,20 +289,38 @@ public void setLastStreamLoadTime(long
lastStreamLoadTime) {
this.backendStatus.lastStreamLoadTime = lastStreamLoadTime;
}
+ // ATTN: This method only the value of "isQueryDisabled",
Review Comment:
Incomplete sentence in comment. Should be 'This method only returns the
value of \"isQueryDisabled\"'.
```suggestion
// ATTN: This method only returns the value of "isQueryDisabled",
```
##########
fe/fe-core/src/main/java/org/apache/doris/system/Backend.java:
##########
@@ -293,20 +289,38 @@ public void setLastStreamLoadTime(long
lastStreamLoadTime) {
this.backendStatus.lastStreamLoadTime = lastStreamLoadTime;
}
+ // ATTN: This method only the value of "isQueryDisabled",
+ // it does not determine the backend IS queryable or not, use
isQueryAvailable instead.
public boolean isQueryDisabled() {
return backendStatus.isQueryDisabled;
}
- public void setQueryDisabled(boolean isQueryDisabled) {
- this.backendStatus.isQueryDisabled = isQueryDisabled;
+ // return true if be status is changed
+ public boolean setQueryDisabled(boolean isQueryDisabled) {
+ if (this.backendStatus.isQueryDisabled != isQueryDisabled) {
+ this.backendStatus.isQueryDisabled = isQueryDisabled;
+ return true;
+ }
+ return false;
}
+ // ATTN: This method only the value of "isLoadDisabled",
Review Comment:
Incomplete sentence in comment. Should be 'This method only returns the
value of \"isLoadDisabled\"'.
```suggestion
// ATTN: This method only returns the value of "isLoadDisabled",
```
##########
fe/fe-core/src/main/java/org/apache/doris/DorisFE.java:
##########
@@ -236,11 +258,15 @@ public static void start(String dorisHomeDir, String
pidDir, String[] args, Star
ThreadPoolManager.registerAllThreadPoolMetric();
startMonitor();
+
+ serverReady.set(true);
+ // JVM will exit when shutdown hook is completed
while (true) {
Thread.sleep(2000);
}
+ LOG.info("Doris FE main loop exited, shutting down gracefully...");
Review Comment:
This log statement is unreachable because the while loop on line 264 is
infinite. The code will never reach line 267.
##########
fe/fe-core/src/main/java/org/apache/doris/DorisFE.java:
##########
@@ -236,11 +258,15 @@ public static void start(String dorisHomeDir, String
pidDir, String[] args, Star
ThreadPoolManager.registerAllThreadPoolMetric();
startMonitor();
+
+ serverReady.set(true);
+ // JVM will exit when shutdown hook is completed
while (true) {
Thread.sleep(2000);
}
+ LOG.info("Doris FE main loop exited, shutting down gracefully...");
} catch (Throwable e) {
- // Some exception may thrown before LOG is inited.
+ // Some exception may throw before LOG is inited.
Review Comment:
Grammatically incorrect: 'may throw' should be 'may be thrown'.
```suggestion
// Some exception may be thrown before LOG is inited.
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -906,19 +911,21 @@ private void handleQueryWithRetry(TUniqueId queryId)
throws Exception {
LOG.info("Query {} finished",
DebugUtil.printId(context.queryId));
break;
} catch (RpcException | UserException e) {
- if (Config.isCloudMode() &&
e.getMessage().contains(FeConstants.CLOUD_RETRY_E230)) {
+ if (Config.isCloudMode() &&
SystemInfoService.needRetryWithReplan(e.getMessage())) {
+ // For errors in SystemInfoService.NEED_REPLAN_ERRORS,
+ // throw exception directly to trigger a replan retry
outside(in StmtExecutor.queryRetry())
throw e;
}
// If the previous try is timeout or cancelled, then do not
need try again.
if (this.coord != null && (this.coord.isQueryCancelled() ||
this.coord.isTimeout())) {
throw e;
}
- LOG.warn("due to exception {} retry {} rpc {} user {}",
+ LOG.warn("retry due to exception {}. retried {} times. is rpc
error: {}, is user error: {}.",
e.getMessage(), i, e instanceof RpcException, e
instanceof UserException);
boolean isNeedRetry = false;
if (Config.isCloudMode()) {
- // cloud mode retry
+ // cloud mode retryF
Review Comment:
Typo in comment: 'retryF' should be 'retry'.
```suggestion
// cloud mode retry
```
##########
fe/fe-core/src/main/java/org/apache/doris/system/Backend.java:
##########
@@ -990,6 +1004,8 @@ public class BackendStatus {
public volatile boolean isLoadDisabled = false;
@SerializedName("isActive")
public volatile boolean isActive = true;
+ @SerializedName("isShutdown")
+ public volatile boolean isShutdown = true;
Review Comment:
The default value of `isShutdown` should be `false`, not `true`. A newly
created backend should not be marked as shutdown by default.
```suggestion
public volatile boolean isShutdown = false;
```
--
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]
