You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@inlong.apache.org by GitBox <gi...@apache.org> on 2023/01/05 11:26:09 UTC
[GitHub] [inlong] gosonzhang opened a new pull request, #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
gosonzhang opened a new pull request, #7170:
URL: https://github.com/apache/inlong/pull/7170
- Fixes #7169
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063055732
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
API calls will be more frequent. If every request is logged, the manager node will explode due to too much log output
Then the exception of this block is the json encoding and decoding operation of the extParams content. If it fails, the caller will provide the specific content.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang merged pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
gosonzhang merged PR #7170:
URL: https://github.com/apache/inlong/pull/7170
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063239942
##########
inlong-manager/manager-web/src/main/java/org/apache/inlong/manager/web/controller/openapi/OpenDataNodeController.java:
##########
@@ -71,24 +70,21 @@ public Response<List<DataNodeInfo>> list(@RequestBody DataNodePageRequest reques
@PostMapping(value = "/node/save")
@ApiOperation(value = "Save node")
@OperationLog(operation = OperationType.CREATE)
- @RequiresRoles(value = UserRoleCode.ADMIN)
- public Response<Integer> save(@Validated @RequestBody DataNodeRequest request) {
+ public Response<Integer> save(@Validated(SaveValidation.class) @RequestBody DataNodeRequest request) {
return Response.success(dataNodeService.save(request, LoginUserUtils.getLoginUser()));
}
@PostMapping(value = "/node/update")
- @OperationLog(operation = OperationType.UPDATE)
@ApiOperation(value = "Update data node")
- @RequiresRoles(value = UserRoleCode.ADMIN)
Review Comment:
Okay.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063173081
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
I did not understand that "If it fails, the caller will provide the specific content."
This extParams parameter is saved in the DB. In case of wrong saving in the DB, or the target conversion class changes, which leads to parsing failure, can you get the specific information of extParams from "e.getMessage"?
If you can't get it, you need to check the specific information in the DB according to the previous series of logs. This is undoubtedly more difficult.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063262599
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
You can simulate and throw an exception directly at the above modification point, to see if the error message of the exception can be obtained in the response
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063261064
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
I think our discussion may be a little inconsistent.
**It is correct to throw an exception, but my question is, can "e.getMessage" throw the details of the error to the caller?**
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063052302
##########
inlong-manager/manager-web/src/main/java/org/apache/inlong/manager/web/controller/openapi/OpenDataNodeController.java:
##########
@@ -71,24 +70,21 @@ public Response<List<DataNodeInfo>> list(@RequestBody DataNodePageRequest reques
@PostMapping(value = "/node/save")
@ApiOperation(value = "Save node")
@OperationLog(operation = OperationType.CREATE)
- @RequiresRoles(value = UserRoleCode.ADMIN)
- public Response<Integer> save(@Validated @RequestBody DataNodeRequest request) {
+ public Response<Integer> save(@Validated(SaveValidation.class) @RequestBody DataNodeRequest request) {
return Response.success(dataNodeService.save(request, LoginUserUtils.getLoginUser()));
}
@PostMapping(value = "/node/update")
- @OperationLog(operation = OperationType.UPDATE)
@ApiOperation(value = "Update data node")
- @RequiresRoles(value = UserRoleCode.ADMIN)
Review Comment:
Excuse me, but why remove the roles check?
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063051079
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
Does the `e.getMessage()` show the detail of the parse error? Such as which field was parsed failed.
If not, then I think this error log is necessary.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063055036
##########
inlong-manager/manager-web/src/main/java/org/apache/inlong/manager/web/controller/openapi/OpenDataNodeController.java:
##########
@@ -71,24 +70,21 @@ public Response<List<DataNodeInfo>> list(@RequestBody DataNodePageRequest reques
@PostMapping(value = "/node/save")
@ApiOperation(value = "Save node")
@OperationLog(operation = OperationType.CREATE)
- @RequiresRoles(value = UserRoleCode.ADMIN)
- public Response<Integer> save(@Validated @RequestBody DataNodeRequest request) {
+ public Response<Integer> save(@Validated(SaveValidation.class) @RequestBody DataNodeRequest request) {
return Response.success(dataNodeService.save(request, LoginUserUtils.getLoginUser()));
}
@PostMapping(value = "/node/update")
- @OperationLog(operation = OperationType.UPDATE)
@ApiOperation(value = "Update data node")
- @RequiresRoles(value = UserRoleCode.ADMIN)
Review Comment:
The role check is performed in the open api, and this will be strengthened in the future to make it more detailed
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang commented on a diff in pull request #7170: [INLONG-7169][Manager] Optimize OpenDataNodeController implementation
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on code in PR #7170:
URL: https://github.com/apache/inlong/pull/7170#discussion_r1063178361
##########
inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/node/es/ElasticsearchDataNodeDTO.java:
##########
@@ -85,8 +85,8 @@ public static ElasticsearchDataNodeDTO getFromJson(@NotNull String extParams) {
try {
return JsonUtils.parseObject(extParams, ElasticsearchDataNodeDTO.class);
} catch (Exception e) {
- LOGGER.error("Failed to extract additional parameters for Elasticsearch data node: ", e);
Review Comment:
The modification of this place is to allow the caller to analyze the problem, rather than the system administrator to go to the log to analyze the problem
Specifically for this error, the error is the parameter decoding error of extParams, mainly two types, one is that the data passed by the caller is not in json format, and the other is the content of the class that does not match the class specified by the parser.
As the caller, it is clear what content is passed. Based on this basis, the error code and error information provided by the assistant are very clear about which field has an error. Through the specified parsing error information and the original data of the call, the call The operator can analyze the problem by himself, without the need for the system maintainer to view the log, that is, this log output is unnecessary.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org