[Enhancement] Add cluster idle HTTP api #53850
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gengjun-git
changed the title
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| if (warehouse == null) { | ||
| throw ErrorReportException.report(ErrorCode.ERR_UNKNOWN_WAREHOUSE, String.format("name: %s", warehouseName)); | ||
| } | ||
| Warehouse warehouse = getWarehouse(warehouseName); | ||
|
|
||
| try { | ||
| long workerGroupId = selectWorkerGroupInternal(warehouse.getId()).orElse(StarOSAgent.DEFAULT_WORKER_GROUP_ID); |
There was a problem hiding this comment.
The most risky bug in this code is:
Potential null pointer dereference due to the use of getWarehouse which might return null.
You can modify the code like this:
public Warehouse getWarehouse(String warehouseName) {
Warehouse warehouse = nameToWh.get(warehouseName);
if (warehouse == null) {
throw ErrorReportException.report(ErrorCode.ERR_UNKNOWN_WAREHOUSE, String.format("name: %s", warehouseName));
}
return warehouse;
}
public Warehouse getWarehouse(long warehouseId) {
Warehouse warehouse = idToWh.get(warehouseId);
if (warehouse == null) {
throw ErrorReportException.report(ErrorCode.ERR_UNKNOWN_WAREHOUSE, String.format("id: %d", warehouseId));
}
return warehouse;
}This ensures that Warehouse is not null before proceeding with operations on it, thus preventing potential null pointer exceptions.
fe/fe-core/src/main/java/com/starrocks/warehouse/WarehouseIdleChecker.java
Show resolved
Hide resolved
gengjun-git
force-pushed
the
add_cluster_idle_api
branch
3 times, most recently
from
40a1c79 to
3294a0a
Compare
) Signed-off-by: shuming.li <[email protected]>
gengjun-git
force-pushed
the
add_cluster_idle_api
branch
from
930162c to
f9df7a8
Compare
HangyuanLiu
reviewed
Dec 16, 2024
| @@ -396,6 +397,7 @@ protected void runRunningJob() throws AlterCancelException { | |||
| this.finishedTimeMs = System.currentTimeMillis(); | |||
|
|
|||
| GlobalStateMgr.getCurrentState().getEditLog().logAlterJob(this); | |||
| WarehouseIdleChecker.updateJobLastFinishTime(warehouseId); | |||
There was a problem hiding this comment.
Is it a better choice to put it in AlterJobV2::run? Then you don't need to write each job separately.
| import com.starrocks.warehouse.IdleStatus; | ||
| import io.netty.handler.codec.http.HttpMethod; | ||
|
|
||
| public class IdleAction extends RestBaseAction { |
There was a problem hiding this comment.
Please indicate what the Idle content is. The name of this class cannot express the meaning of the content.
| @@ -146,10 +151,7 @@ public List<Long> getAllComputeNodeIds(long warehouseId) { | |||
| } | |||
|
|
|||
| private List<Long> getAllComputeNodeIds(long warehouseId, long workerGroupId) { | |||
| Warehouse warehouse = idToWh.get(warehouseId); | |||
There was a problem hiding this comment.
Why do we need to delete the existence check of warehouse?
There was a problem hiding this comment.
There is check in the getWarehouse function
Signed-off-by: gengjun-git <[email protected]>
Signed-off-by: gengjun-git <[email protected]>
Signed-off-by: gengjun-git <[email protected]>
Seaven
previously approved these changes
Dec 16, 2024
stephen-shelby
previously approved these changes
Dec 16, 2024
kevincai
reviewed
Dec 16, 2024
| @@ -2755,6 +2755,9 @@ public class Config extends ConfigBase { | |||
| @ConfField(mutable = true) | |||
| public static int lake_warehouse_max_compute_replica = 3; | |||
|
|
|||
| @ConfField(mutable = true, comment = "time interval to check whether warehouse is idle") | |||
| public static long warehouse_idle_check_interval_seconds = 60; | |||
There was a problem hiding this comment.
consider set this to 0 in open source version to disable the check by default?
There was a problem hiding this comment.
Add a new config warehouse_idle_check_enable, because the warehouse_idle_check_interval_seconds is also used to check the last job finish time.
| } | ||
|
|
||
| @Override | ||
| public void execute(BaseRequest request, BaseResponse response) { |
There was a problem hiding this comment.
should this api endpoint be protected by authentication?
There was a problem hiding this comment.
Not need, there is no secret info
There was a problem hiding this comment.
could be a security concern but I would defer it to you guys decision.
Comment on lines
67
to
73
| if (runningSQL.get() == 0 | ||
| && runningStreamLoad == 0 | ||
| && runningBrokerSparkLoad == 0 | ||
| && runningRoutineLoad == 0 | ||
| && runningBackupRestore == 0 | ||
| && runningAlterJob == 0 | ||
| && runningTask == 0 |
There was a problem hiding this comment.
sum them , check the sum == 0?
nshangyiming
previously approved these changes
Dec 16, 2024
HangyuanLiu
previously approved these changes
Dec 16, 2024
Signed-off-by: gengjun-git <[email protected]>
Signed-off-by: gengjun-git <[email protected]>
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 123 / 228 (53.95%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@Mergifyio backport branch-3.4 |
|
@Mergifyio backport branch-3.3 |
|
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Dec 17, 2024
(cherry picked from commit 6cd9fbc) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/common/Config.java # fe/fe-core/src/main/java/com/starrocks/statistic/HyperStatisticsCollectJob.java
18 tasks
mergify bot
pushed a commit
that referenced
this pull request
Dec 17, 2024
(cherry picked from commit 6cd9fbc) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/common/Config.java # fe/fe-core/src/main/java/com/starrocks/qe/feedback/PlanAdvisorExecutor.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/main/java/com/starrocks/statistic/HyperStatisticsCollectJob.java
mergify bot
pushed a commit
that referenced
this pull request
Dec 17, 2024
(cherry picked from commit 6cd9fbc) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/alter/LakeTableSchemaChangeJob.java # fe/fe-core/src/main/java/com/starrocks/alter/OnlineOptimizeJobV2.java # fe/fe-core/src/main/java/com/starrocks/alter/RollupJobV2.java # fe/fe-core/src/main/java/com/starrocks/alter/SchemaChangeJobV2.java # fe/fe-core/src/main/java/com/starrocks/backup/BackupJob.java # fe/fe-core/src/main/java/com/starrocks/backup/RestoreJob.java # fe/fe-core/src/main/java/com/starrocks/common/Config.java # fe/fe-core/src/main/java/com/starrocks/connector/metadata/MetadataExecutor.java # fe/fe-core/src/main/java/com/starrocks/datacache/DataCacheSelectExecutor.java # fe/fe-core/src/main/java/com/starrocks/load/routineload/RoutineLoadJob.java # fe/fe-core/src/main/java/com/starrocks/load/streamload/StreamLoadMgr.java # fe/fe-core/src/main/java/com/starrocks/load/streamload/StreamLoadTask.java # fe/fe-core/src/main/java/com/starrocks/qe/ConnectContext.java # fe/fe-core/src/main/java/com/starrocks/qe/StmtExecutor.java # fe/fe-core/src/main/java/com/starrocks/qe/feedback/PlanAdvisorExecutor.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/main/java/com/starrocks/server/WarehouseManager.java # fe/fe-core/src/main/java/com/starrocks/sql/util/CustomizedQueryExecutor.java # fe/fe-core/src/main/java/com/starrocks/statistic/HyperStatisticsCollectJob.java # fe/fe-core/src/test/java/com/starrocks/scheduler/MVRefreshTestBase.java # fe/fe-core/src/test/java/com/starrocks/sql/optimizer/rule/transformation/materialization/MvRewriteTestBase.java
This was referenced Dec 17, 2024
wanpengfei-git
pushed a commit
that referenced
this pull request
Dec 17, 2024
Signed-off-by: gengjun-git <[email protected]> Co-authored-by: gengjun-git <[email protected]>
wanpengfei-git
pushed a commit
that referenced
this pull request
Dec 19, 2024
Signed-off-by: gengjun-git <[email protected]> Co-authored-by: gengjun-git <[email protected]>
|
ignore backport check: 3.2.14 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
Add cluster idle api to help judge the cluster status.
What I'm doing:
Add a new daemon thread WarehouseIdleChecker to count all the tasks being executed and the time when the last task ended. If all the tasks being executed are 0 and the time when the last task ended has exceeded
Config.warehouse_idle_check_interval_seconds, the system is considered idle.The entry point for all SQL execution in the system is StmtExecutor. If it is synchronous SQL, we only need to count the SQL executed in StmtExecutor. If it is asynchronous SQL, such as broker load, we also need to count these asynchronously executed tasks: Stream Load, Broker Load, Spark Load, Routine Load, Backup/Restore, Schema Change.
In addition, for SQL executed within the system, such as statistics collection, we do not need to count them. Add the
isInternalfield in StmtExecutor to determine whether the SQL is initiated internally or by the user.api request
/api/idle_statusapi response
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: