[ISSUE#12994]Optimize config operation. #13002
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
|
|
|
|
|
|
|
Ok
Em qua., 1 de jan. de 2025, 10:20, wuyfee ***@***.***>
escreveu:
… $\color{green}{SUCCESS}$
DETAILS <https://github.com/alibaba/nacos/actions/runs/12570842757>
✅ - docker: success
✅ - deploy (standalone & cluster & standalone_auth): success
✅ - e2e-java-test (standalone & cluster & standalone_auth): success
✅ - e2e-go-test (standalone & cluster): success
✅ - e2e-cpp-test (standalone & cluster): success
✅ - e2e-csharp-test (standalone & cluster): success
✅ - e2e-nodejs-test (standalone & cluster): success
✅ - e2e-python-test (standalone & cluster): success
✅ - clean (standalone & cluster & standalone_auth): success
—
Reply to this email directly, view it on GitHub
<#13002 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/A4MT7RHTE4HIKQGBB6BTEHD2IPTQDAVCNFSM6AAAAABUMCMBGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRXGAYDOMRUHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
change capacity aspect cutpoint to ConfigOperationService |
|
Ok
Em dom., 5 de jan. de 2025, 22:41, shiyiyue1102 ***@***.***>
escreveu:
… change capacity aspect cutpoint to ConfigOperationService
—
Reply to this email directly, view it on GitHub
<#13002 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/A4MT7RAQGECCROGKETAINEL2JHNL3AVCNFSM6AAAAABUMCMBGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZRHEZDAMRYHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
|
|
|
|
|
| /** | ||
| * Deletes configuration information based on the IDs list. | ||
| */ | ||
| public Boolean deleteConfigs(List<Long> ids, String srcIp, String srcUser) { |
There was a problem hiding this comment.
deleteConfigs should check capacity , or can we split deleteConfigs operation to serval single deleteConfig operation in ConfigController?
There was a problem hiding this comment.
if we have already split batch operations on controllers, method deteConfigs should be removed.
| /** | ||
| * Batch insert or update configuration information. | ||
| */ | ||
| public Map<String, Object> batchInsertOrUpdate(List<ConfigAllInfo> configInfoList, String srcUser, String srcIp, |
There was a problem hiding this comment.
all batch operation invoked ConfigOperationService single operation interface, all aspects works at single config publish & delete interface of ConfigOperationService
There was a problem hiding this comment.
if we have already split batch operations on controllers, method batchInsertOrUpdate should be removed.
KiteSoar
force-pushed
the
optimize-config-operation
branch
from
8a6e38c to
2fab31d
Compare
|
|
|
|
|
|
KiteSoar
force-pushed
the
optimize-config-operation
branch
from
a48001e to
a1b8222
Compare
|
|
| LOGGER.info("[CapacityManagement] Intercepting publishConfig operation for dataId: {}, group: {}, namespaceId: {}", | ||
| dataId, group, namespaceId); | ||
|
|
||
| if (StringUtils.isBlank(betaIps) && StringUtils.isBlank(tag)) { |
There was a problem hiding this comment.
add skip check for grayName
There was a problem hiding this comment.
ok，but current grayName is set in the publish method, grayName in the aspect before entering the method is all null, unless we set grayName before entering the publish method later.
| LOGGER.info("[CapacityManagement] Intercepting deleteConfig operation for dataId: {}, group: {}, namespaceId: {}", dataId, group, | ||
| namespaceId); | ||
|
|
||
| ConfigInfo configInfo = configInfoPersistService.findConfigInfo(dataId, group, namespaceId); |
There was a problem hiding this comment.
should check gray delete skip here like publish config?
There was a problem hiding this comment.
gray delete will not use this interface. Instead com.alibaba.nacos.config.server.controller.ConfigController#stopBeta
There was a problem hiding this comment.
ok. I just saw the changes of this PR develop-optimize-table #13119.
| // Execute whether to callback based on the sql operation result. | ||
| doResult(counterMode, response, group, tenant, result, hasTenant); | ||
| return result; | ||
| return pjp.proceed(); |
There was a problem hiding this comment.
should rollback capacity when proceed return false?
There was a problem hiding this comment.
Yes, in the case of batch import, false may be returned.
| @@ -376,34 +381,11 @@ private boolean isOverSize(String group, String tenant, int currentSize, int max | |||
| return false; | |||
| } | |||
|
|
|||
| private void doResult(CounterMode counterMode, HttpServletResponse response, String group, String tenant, | |||
There was a problem hiding this comment.
result check has been deleted?
There was a problem hiding this comment.
because the aspect was acting on the ConfigController#publishConfig method before, the HttpServletResponse object could be obtained. However, after changing to the configOperationService#publishConfig method, the HttpServletResponse object cannot be obtained.
There was a problem hiding this comment.
because the aspect was acting on the ConfigController#publishConfig method before, the HttpServletResponse object could be obtained. However, after changing to the configOperationService#publishConfig method, the HttpServletResponse object cannot be obtained.
but we should rollback capacity when returning false or throw exception
There was a problem hiding this comment.
Change to this，capacity can be rolled back when returning false or throwing an exception.
private Object getResult(ProceedingJoinPoint pjp, String group, String namespaceId, CounterMode counterMode, boolean hasTenant) throws Throwable {
try {
// Execute operation actually.
Boolean result = (Boolean) pjp.proceed();
if (!result) {
rollbackUsage(counterMode, group, namespaceId, hasTenant);
}
return result;
} catch (Throwable throwable) {
LOGGER.warn("[capacityManagement] inner operation throw exception, rollback, group: {}, namespaceId: {}", group,
namespaceId, throwable);
rollbackUsage(counterMode, group, namespaceId, hasTenant);
throw throwable;
}
}
| final String configTags = configForm.getConfigTags(); | ||
| final String requestIpApp = configRequestInfo.getRequestIpApp(); | ||
| final String scrIp = configRequestInfo.getSrcIp(); | ||
| final String scrType = configRequestInfo.getSrcType(); |
There was a problem hiding this comment.
config change aspect should distinguish beta, tag ,gray ?
There was a problem hiding this comment.
I think it's okay. It can be like this.
String grayName = null;
String grayRuleExp = null;
if (StringUtils.isNotBlank(betaIps)) {
grayName = BetaGrayRule.TYPE_BETA;
grayRuleExp = betaIps;
} else if (StringUtils.isNotBlank(tag)) {
grayName = TagGrayRule.TYPE_TAG + "_" + configForm.getTag();
grayRuleExp = tag;
}
| */ | ||
| @DeleteMapping(params = "delType=ids") | ||
| @Secured(action = ActionTypes.WRITE, signType = SignType.CONFIG) | ||
| public RestResult<Boolean> deleteConfigs(HttpServletRequest request, @RequestParam(value = "ids") List<Long> ids) { | ||
| if (CollectionUtils.isEmpty(ids)) { | ||
| return RestResultUtils.failed("The ids cannot be empty."); | ||
| } |
There was a problem hiding this comment.
empty check here is no need , param is required for default
There was a problem hiding this comment.
Yes, I will correct
| String persistEvent = ConfigTraceService.PERSISTENCE_EVENT; | ||
| String grayName = ""; | ||
| if (StringUtils.isBlank(tag)) { | ||
| configInfoPersistService.removeConfigInfo(dataId, group, namespaceId, clientIp, srcUser); | ||
| } else { | ||
| persistEvent = ConfigTraceService.PERSISTENCE_EVENT_TAG + "-" + tag; | ||
| persistEvent = ConfigTraceService.PERSISTENCE_EVENT_TAG + "_" + tag; |
There was a problem hiding this comment.
why changing split "-" to "_"?
There was a problem hiding this comment.
This is when the deletion configuration method on the SDK side is changed to call the deletion method of ConfigOperationService. In order to maintain consistency with the original format on the SDK side, as shown in the following figure.
There was a problem hiding this comment.
all right , if delete config using "" and publish config using "-", it will be more proper to change "" to "-" in delete config.
There was a problem hiding this comment.
done，Has been changed to "-".
| @@ -70,6 +70,8 @@ public class ConfigForm implements Serializable { | |||
|
|
|||
| private String schema; | |||
|
|
|||
| private Boolean updateForExist = Boolean.TRUE; | |||
There was a problem hiding this comment.
it seems that moving this param to ConfigRequestInfo may be more proper
There was a problem hiding this comment.
My understanding is updateForExist means whether to update when the configuration already exists. It should be part of business logic. The ConfigRequestInfo class belong to request metadata. Irrelevant to business operations，so it seems appropriate to put it in Conform？
There was a problem hiding this comment.
As you said, "updateForExist means whether to update when the configuration already exists", it is not a part of config itself. Conform contains things about config.
There was a problem hiding this comment.
ok, it has been moved to the ConfigRequestInfo class.
|
|
KiteSoar
force-pushed
the
optimize-config-operation
branch
from
3c22682 to
1abdaa8
Compare
|
|
| if (limitType != null) { | ||
| return response4Limit(request, response, limitType); | ||
| throw new NacosException(ErrorCode.SERVER_ERROR.getCode(), |
There was a problem hiding this comment.
should return different error code for different limit type
|
|
KiteSoar
force-pushed
the
optimize-config-operation
branch
from
572a3eb to
1b9d0be
Compare
|
|
KiteSoar
force-pushed
the
optimize-config-operation
branch
from
1b9d0be to
eb7cfcc
Compare
|
|
What is the purpose of the change
its close #12994
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.