You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "levy5307 (via GitHub)" <gi...@apache.org> on 2023/04/24 06:29:29 UTC
[GitHub] [doris] levy5307 opened a new pull request, #18990: Alter resource group
levy5307 opened a new pull request, #18990:
URL: https://github.com/apache/doris/pull/18990
# Proposed changes
Issue Number: https://github.com/apache/doris/issues/18098
## Problem summary
```
ALTER RESOURCE GROUP group_name PROPERTIES (
"cpu_share"="1"
);
```
## Checklist(Required)
* [ ] Does it affect the original behavior
* [x] Has unit tests been added
* [ ] Has document been added or modified
* [ ] Does it need to update dependencies
* [x] Is this PR support rollback (If NO, please explain WHY)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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
[GitHub] [doris] wangbo commented on a diff in pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "wangbo (via GitHub)" <gi...@apache.org>.
wangbo commented on code in PR #18990:
URL: https://github.com/apache/doris/pull/18990#discussion_r1178541166
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroupMgr.java:
##########
@@ -151,10 +155,30 @@ public void createResourceGroup(CreateResourceGroupStmt stmt) throws DdlExceptio
LOG.info("Create resource group success: {}", resourceGroup);
}
- public void dropResourceGroup(DropResourceGroupStmt stmt) throws DdlException {
- if (!Config.enable_resource_group) {
- throw new DdlException("unsupported feature now, coming soon.");
+ public void alterResourceGroup(AlterResourceGroupStmt stmt) throws DdlException {
+ checkResourceGroupEnabled();
+
+ String resourceGroupName = stmt.getResourceGroupName();
+ Map<String, String> properties = stmt.getProperties();
+ ResourceGroup newResourceGroup;
+ writeLock();
+ try {
+ if (!nameToResourceGroup.containsKey(resourceGroupName)) {
+ throw new DdlException("Resource Group(" + resourceGroupName + ") does not exist.");
+ }
+ ResourceGroup resourceGroup = nameToResourceGroup.get(resourceGroupName);
+ newResourceGroup = ResourceGroup.create(resourceGroup, properties);
+ nameToResourceGroup.put(resourceGroupName, newResourceGroup);
Review Comment:
It seems that currently we not support partial attribute alter group.
Need a todo here/
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroupMgr.java:
##########
@@ -151,10 +155,30 @@ public void createResourceGroup(CreateResourceGroupStmt stmt) throws DdlExceptio
LOG.info("Create resource group success: {}", resourceGroup);
}
- public void dropResourceGroup(DropResourceGroupStmt stmt) throws DdlException {
- if (!Config.enable_resource_group) {
- throw new DdlException("unsupported feature now, coming soon.");
+ public void alterResourceGroup(AlterResourceGroupStmt stmt) throws DdlException {
+ checkResourceGroupEnabled();
+
+ String resourceGroupName = stmt.getResourceGroupName();
+ Map<String, String> properties = stmt.getProperties();
+ ResourceGroup newResourceGroup;
+ writeLock();
+ try {
+ if (!nameToResourceGroup.containsKey(resourceGroupName)) {
+ throw new DdlException("Resource Group(" + resourceGroupName + ") does not exist.");
+ }
+ ResourceGroup resourceGroup = nameToResourceGroup.get(resourceGroupName);
+ newResourceGroup = ResourceGroup.create(resourceGroup, properties);
+ nameToResourceGroup.put(resourceGroupName, newResourceGroup);
Review Comment:
It seems that currently we not support partial attribute alter group.
Need a todo here.
--
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
[GitHub] [doris] levy5307 commented on pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on PR #18990:
URL: https://github.com/apache/doris/pull/18990#issuecomment-1519535206
run buildall
--
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
[GitHub] [doris] luozenglin merged pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "luozenglin (via GitHub)" <gi...@apache.org>.
luozenglin merged PR #18990:
URL: https://github.com/apache/doris/pull/18990
--
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
[GitHub] [doris] levy5307 commented on a diff in pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on code in PR #18990:
URL: https://github.com/apache/doris/pull/18990#discussion_r1177284632
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -884,8 +886,11 @@ public void analyze(TQueryOptions tQueryOptions) throws UserException {
|| parsedStmt instanceof InsertStmt
|| parsedStmt instanceof CreateTableAsSelectStmt) {
if (Config.enable_resource_group && context.sessionVariable.enablePipelineEngine()) {
- analyzer.setResourceGroups(analyzer.getEnv().getResourceGroupMgr()
- .getResourceGroup(context.sessionVariable.resourceGroup));
+ ResourceGroup resourceGroup = analyzer.getEnv().getResourceGroupMgr().getResourceGroup(
+ context.sessionVariable.resourceGroup);
+ List<TPipelineResourceGroup> resourceGroups = Lists.newArrayList();
+ resourceGroups.add(resourceGroup.toThrift());
Review Comment:
So we should write another func named with `getResourceGroupThrift` to get return value with type of ` List<TPipelineResourceGroup>`. What do you think?
--
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
[GitHub] [doris] github-actions[bot] commented on pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18990:
URL: https://github.com/apache/doris/pull/18990#issuecomment-1524519865
PR approved by anyone and no changes requested.
--
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
[GitHub] [doris] luozenglin commented on a diff in pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "luozenglin (via GitHub)" <gi...@apache.org>.
luozenglin commented on code in PR #18990:
URL: https://github.com/apache/doris/pull/18990#discussion_r1175010148
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroup.java:
##########
@@ -88,6 +89,13 @@ private static void checkProperties(Map<String, String> properties) throws DdlEx
}
}
+ public void modifyProperties(Map<String, String> properties) throws DdlException {
Review Comment:
need to update version
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroupMgr.java:
##########
@@ -151,6 +147,27 @@ public void createResourceGroup(CreateResourceGroupStmt stmt) throws DdlExceptio
LOG.info("Create resource group success: {}", resourceGroup);
}
+ public void alterResourceGroup(AlterResourceGroupStmt stmt) throws DdlException {
+ if (!Config.enable_resource_group) {
+ throw new DdlException("unsupported feature now, coming soon.");
+ }
+
+ String resourceGroupName = stmt.getResourceGroupName();
+ Map<String, String> properties = stmt.getProperties();
+ writeLock();
+ try {
+ if (!nameToResourceGroup.containsKey(resourceGroupName)) {
+ throw new DdlException("Resource Group(" + resourceGroupName + ") does not exist.");
+ }
+ ResourceGroup resourceGroup = nameToResourceGroup.get(resourceGroupName);
+ resourceGroup.modifyProperties(properties);
+ Env.getCurrentEnv().getEditLog().logAlterResourceGroup(resourceGroup);
+ LOG.info("Alter resource success. Resource Group: {}", resourceGroup);
Review Comment:
The log should be placed outside of the lock
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -884,8 +886,11 @@ public void analyze(TQueryOptions tQueryOptions) throws UserException {
|| parsedStmt instanceof InsertStmt
|| parsedStmt instanceof CreateTableAsSelectStmt) {
if (Config.enable_resource_group && context.sessionVariable.enablePipelineEngine()) {
- analyzer.setResourceGroups(analyzer.getEnv().getResourceGroupMgr()
- .getResourceGroup(context.sessionVariable.resourceGroup));
+ ResourceGroup resourceGroup = analyzer.getEnv().getResourceGroupMgr().getResourceGroup(
+ context.sessionVariable.resourceGroup);
+ List<TPipelineResourceGroup> resourceGroups = Lists.newArrayList();
+ resourceGroups.add(resourceGroup.toThrift());
Review Comment:
How to ensure thread safety of resourceGroup when multiple threads are executing toThrift and alterResourceGroup?
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroupMgr.java:
##########
@@ -151,6 +147,27 @@ public void createResourceGroup(CreateResourceGroupStmt stmt) throws DdlExceptio
LOG.info("Create resource group success: {}", resourceGroup);
}
+ public void alterResourceGroup(AlterResourceGroupStmt stmt) throws DdlException {
+ if (!Config.enable_resource_group) {
Review Comment:
It might be better to abstract this check into a method
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroupMgr.java:
##########
@@ -91,20 +90,17 @@ public void init() {
}
}
- public List<TPipelineResourceGroup> getResourceGroup(String groupName) throws UserException {
- List<TPipelineResourceGroup> resourceGroups = Lists.newArrayList();
+ public ResourceGroup getResourceGroup(String groupName) throws UserException {
Review Comment:
need to return a list, because later may support resource group nesting, need to return all ancestor resource group
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AlterResourceGroupStmt.java:
##########
@@ -0,0 +1,73 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.analysis;
+
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.common.UserException;
+import org.apache.doris.common.util.PrintableMap;
+import org.apache.doris.mysql.privilege.PrivPredicate;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.resource.resourcegroup.ResourceGroup;
+
+import java.util.Map;
+
+public class AlterResourceGroupStmt extends DdlStmt {
+ private final String resourceGroupName;
+ private final Map<String, String> properties;
+
+ public AlterResourceGroupStmt(String resourceGroupName, Map<String, String> properties) {
+ this.resourceGroupName = resourceGroupName;
+ this.properties = properties;
+ }
+
+ public String getResourceGroupName() {
+ return resourceGroupName;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ @Override
+ public void analyze(Analyzer analyzer) throws UserException {
+ super.analyze(analyzer);
+
+ // check auth
+ if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(), PrivPredicate.ADMIN)) {
+ ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "ADMIN");
+ }
+
+ if (properties == null || properties.isEmpty()) {
+ throw new AnalysisException("Resource group properties can't be null");
+ }
+
+ ResourceGroup resourceGroup = Env.getCurrentEnv().getResourceGroupMgr().getResourceGroup(resourceGroupName);
+ resourceGroup.checkProperties(properties);
Review Comment:
The current `checkProperties` checks if the `properties` have all the required properties, but it is not needed for here.
--
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
[GitHub] [doris] github-actions[bot] commented on pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18990:
URL: https://github.com/apache/doris/pull/18990#issuecomment-1524519793
PR approved by at least one committer and no changes requested.
--
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
[GitHub] [doris] levy5307 commented on a diff in pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on code in PR #18990:
URL: https://github.com/apache/doris/pull/18990#discussion_r1177417779
##########
fe/fe-core/src/main/java/org/apache/doris/resource/resourcegroup/ResourceGroup.java:
##########
@@ -88,6 +89,13 @@ private static void checkProperties(Map<String, String> properties) throws DdlEx
}
}
+ public void modifyProperties(Map<String, String> properties) throws DdlException {
Review Comment:
Done
--
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
[GitHub] [doris] levy5307 commented on pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on PR #18990:
URL: https://github.com/apache/doris/pull/18990#issuecomment-1523262509
run buildall
--
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
[GitHub] [doris] levy5307 commented on a diff in pull request #18990: [Feature](resource-group) Support alter resource group
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on code in PR #18990:
URL: https://github.com/apache/doris/pull/18990#discussion_r1177284632
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -884,8 +886,11 @@ public void analyze(TQueryOptions tQueryOptions) throws UserException {
|| parsedStmt instanceof InsertStmt
|| parsedStmt instanceof CreateTableAsSelectStmt) {
if (Config.enable_resource_group && context.sessionVariable.enablePipelineEngine()) {
- analyzer.setResourceGroups(analyzer.getEnv().getResourceGroupMgr()
- .getResourceGroup(context.sessionVariable.resourceGroup));
+ ResourceGroup resourceGroup = analyzer.getEnv().getResourceGroupMgr().getResourceGroup(
+ context.sessionVariable.resourceGroup);
+ List<TPipelineResourceGroup> resourceGroups = Lists.newArrayList();
+ resourceGroups.add(resourceGroup.toThrift());
Review Comment:
So we should write another func named with `getResourceGroupThrift` to get return value with type of ` List<TPipelineResourceGroup>`. What do you think?
--
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