morningman commented on a change in pull request #3519:
URL: https://github.com/apache/incubator-doris/pull/3519#discussion_r422617501
##########
File path: fe/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -4894,8 +4894,7 @@ public void renameTable(Database db, OlapTable table,
TableRenameClause tableRen
// check if rollup has same name
if (table.getType() == TableType.OLAP) {
Review comment:
We can just remove the `if (table.getType() == TableType.OLAP)`. The
argument is already `OlapTable`
##########
File path: fe/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -4894,8 +4894,7 @@ public void renameTable(Database db, OlapTable table,
TableRenameClause tableRen
// check if rollup has same name
if (table.getType() == TableType.OLAP) {
- OlapTable olapTable = (OlapTable) table;
- for (String idxName: olapTable.getIndexNameToId().keySet()) {
+ for (String idxName: table.getIndexNameToId().keySet()) {
Review comment:
```suggestion
for (String idxName : table.getIndexNameToId().keySet()) {
```
##########
File path: fe/src/main/java/org/apache/doris/system/Backend.java
##########
@@ -346,6 +347,23 @@ public double getMaxDiskUsedPct() {
return maxPct;
}
+ public boolean checkDiskExceedLimitByStorageMedium(TStorageMedium
storageMedium) {
+ // no available disk with storage medium
+ if (getDiskNumByStorageMedium(storageMedium) <= 0) {
+ return true;
+ }
+ ImmutableMap<String, DiskInfo> disks = disksRef.get();
+ boolean exceedLimit = false;
+ for (DiskInfo diskInfo : disks.values()) {
Review comment:
Is this logic correct? If all disks are offline or is not with specified
storage medium, then this method will return false?
##########
File path: fe/src/main/java/org/apache/doris/system/Backend.java
##########
@@ -346,6 +347,23 @@ public double getMaxDiskUsedPct() {
return maxPct;
}
+ public boolean checkDiskExceedLimitByStorageMedium(TStorageMedium
storageMedium) {
Review comment:
Add comment to explain the return value of this method.
the name `checkDiskExceedLimitByStorageMedium` is a little bit confused.
##########
File path: fe/src/main/java/org/apache/doris/system/SystemInfoService.java
##########
@@ -443,7 +390,7 @@ public void releaseBackends(String clusterName, boolean
isReplay) {
}
List<List<Backend>> hostList =
Lists.newArrayList(hostBackendsMapInCluster.values());
- Collections.sort(hostList, hostBackendsListComparator);
+ hostList.sort(hostBackendsListComparator);
Review comment:
Actually, the method `calculateDecommissionBackends()` is deprecated.
So could you please add `@Derepcated` to this method?
##########
File path: fe/src/main/java/org/apache/doris/system/SystemInfoService.java
##########
@@ -729,8 +639,18 @@ public void releaseBackends(String clusterName, boolean
isReplay) {
// return null if not enough backend
// use synchronized to run serially
public synchronized List<Long> seqChooseBackendIds(int backendNum, boolean
needAlive, boolean isCreate,
- String clusterName) {
- long lastBackendId = -1L;
+ String clusterName) {
+ return seqChooseBackendIds(backendNum, needAlive, isCreate,
clusterName, getClusterBackends(clusterName));
+ }
+ public synchronized List<Long> seqChooseBackendIdsByStorageMedium(int
backendNum, boolean needAlive, boolean isCreate,
+ String
clusterName, TStorageMedium storageMedium) {
+ final List<Backend> backends =
getClusterBackends(clusterName).stream().filter(v ->
!v.checkDiskExceedLimitByStorageMedium(storageMedium)).collect(Collectors.toList());
Review comment:
This is not user friendly. For example, if all backends not meet the
`checkDiskExceedLimitByStorageMedium()`, the user will only get the msg: "Not
enough backends", but he will not know why there is no enough backend.
So could you please find a way to return the detail msg to user about the
reason of failure?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
