You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@streampark.apache.org by "VampireAchao (via GitHub)" <gi...@apache.org> on 2023/11/04 06:01:18 UTC
[PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
VampireAchao opened a new pull request, #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310
<!--
Thank you for contributing to StreamPark! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
## Contribution Checklist
- If this is your first time, please read our contributor guidelines: [Submit Code](https://streampark.apache.org/community/submit_guide/submit_code).
- Make sure that the pull request corresponds to a [GITHUB issue](https://github.com/apache/incubator-streampark/issues).
- Name the pull request in the form "[Feature] Title of the pull request", where *Feature* can be replaced by `Hotfix`, `Bug`, etc.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- If the PR is unfinished, add `[WIP]` in your PR title, e.g., `[WIP][Feature] Title of the pull request`.
-->
## What changes were proposed in this pull request
Issue Number: see #3064 <!-- REMOVE this line if no issue to close -->
<!--(For example: This pull request proposed to add checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
Improve streampark-console core package module service name base on [3.1 Naming Style]
1. Just remove method
2. Just in service
## Verifying this change
<!--*(Please pick either of the following options)*-->
This change is a trivial rework / code cleanup without any test coverage.
Yes *(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
No *(or)*
This change added tests and can be verified as follows:
<!--*(example:)*
- *Added integration tests for end-to-end.*
- *Added *Test to verify the change.*
- *Manually verified the change by testing locally.* -->
## Does this pull request potentially affect one of the following parts
- Dependencies (does it add or upgrade a dependency): No
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "wolfboys (via GitHub)" <gi...@apache.org>.
wolfboys commented on code in PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#discussion_r1382494071
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationLogServiceImpl.java:
##########
@@ -50,14 +50,13 @@ public IPage<ApplicationLog> getPage(ApplicationLog applicationLog, RestRequest
}
@Override
- public void removeApp(Long appId) {
+ public void removeByAppId(Long appId) {
LambdaQueryWrapper<ApplicationLog> queryWrapper =
new LambdaQueryWrapper<ApplicationLog>().eq(ApplicationLog::getAppId, appId);
this.remove(queryWrapper);
}
- @Override
- public Boolean delete(ApplicationLog applicationLog) {
+ public boolean remove(ApplicationLog applicationLog) {
Review Comment:
We don't need this method.
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/ApplicationLogService.java:
##########
@@ -27,7 +27,7 @@ public interface ApplicationLogService extends IService<ApplicationLog> {
IPage<ApplicationLog> getPage(ApplicationLog applicationLog, RestRequest request);
- void removeApp(Long appId);
+ void removeByAppId(Long appId);
- Boolean delete(ApplicationLog applicationLog);
+ boolean remove(ApplicationLog applicationLog);
Review Comment:
We don't need this method.
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/controller/ApplicationController.java:
##########
@@ -335,7 +335,7 @@ public RestResponse optionlog(ApplicationLog applicationLog, RestRequest request
@PostMapping("deleteOperationLog")
@RequiresPermissions("app:delete")
public RestResponse deleteOperationLog(ApplicationLog applicationLog) {
- Boolean deleted = applicationLogService.delete(applicationLog);
+ Boolean deleted = applicationLogService.removeById(applicationLog);
return RestResponse.success(deleted);
}
Review Comment:
We can improve this method :
```
public RestResponse deleteOperationLog(Long id) {
Boolean deleted = applicationLogService.removeById(id);
return RestResponse.success(deleted);
}
```
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "wolfboys (via GitHub)" <gi...@apache.org>.
wolfboys merged PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP][Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "VampireAchao (via GitHub)" <gi...@apache.org>.
VampireAchao commented on PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#issuecomment-1793359131
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "wolfboys (via GitHub)" <gi...@apache.org>.
wolfboys commented on code in PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#discussion_r1382350496
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/controller/ApplicationController.java:
##########
@@ -344,15 +344,15 @@ public RestResponse deleteOperationLog(ApplicationLog applicationLog) {
@PostMapping("delete")
@RequiresPermissions("app:delete")
public RestResponse delete(Application app) throws InternalException {
- Boolean deleted = applicationManageService.delete(app);
+ Boolean deleted = applicationManageService.removeByApp(app);
Review Comment:
remove ...
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#discussion_r1382343339
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/ApplicationBackUpService.java:
##########
@@ -74,7 +74,7 @@ public interface ApplicationBackUpService extends IService<ApplicationBackUp> {
*
* @param appParam the application to be removed
*/
- void removeApp(Application appParam);
+ void removeByApp(Application appParam);
Review Comment:
Some places are Boolean remove (Application appParam); Could you please check it out?
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/application/ApplicationManageService.java:
##########
@@ -105,7 +105,7 @@ public interface ApplicationManageService extends IService<Application> {
* @param appParam The Application to be deleted.
* @return True if the deletion was successful, false otherwise.
*/
- Boolean delete(Application appParam);
+ Boolean remove(Application appParam);
Review Comment:
this
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#issuecomment-1793361272
cc @RocMarshal
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP][Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#discussion_r1382341972
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/VariableService.java:
##########
@@ -75,7 +75,7 @@ public interface VariableService extends IService<Variable> {
*
* @param variable the Variable object to be deleted
*/
- void deleteVariable(Variable variable);
+ void removeVariable(Variable variable);
Review Comment:
removeByVariable or remove?
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/ApplicationLogService.java:
##########
@@ -27,7 +27,7 @@ public interface ApplicationLogService extends IService<ApplicationLog> {
IPage<ApplicationLog> getPage(ApplicationLog applicationLog, RestRequest request);
- void removeApp(Long appId);
+ void removeByAppId(Long appId);
- Boolean delete(ApplicationLog applicationLog);
+ boolean removeById(ApplicationLog applicationLog);
Review Comment:
removeById？There is no id in the parameters
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/ApplicationLogService.java:
##########
@@ -27,7 +27,7 @@ public interface ApplicationLogService extends IService<ApplicationLog> {
IPage<ApplicationLog> getPage(ApplicationLog applicationLog, RestRequest request);
- void removeApp(Long appId);
+ void removeByAppId(Long appId);
- Boolean delete(ApplicationLog applicationLog);
+ boolean removeById(ApplicationLog applicationLog);
Review Comment:
removeById？There is no id in the parameters
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/controller/FlinkEnvController.java:
##########
@@ -96,7 +96,7 @@ public RestResponse update(FlinkEnv version) throws Exception {
@Operation(summary = "Delete flink environment")
@PostMapping("delete")
public RestResponse delete(Long id) {
- flinkEnvService.delete(id);
+ flinkEnvService.remove(id);
Review Comment:
removeById
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#issuecomment-1793613176
unreported exception InternalException; must be caught or declared to be thrown
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improve] Improve streampark-console core package module service remove method name base on [3.1 Naming Style] [incubator-streampark]
Posted by "VampireAchao (via GitHub)" <gi...@apache.org>.
VampireAchao commented on PR #3310:
URL: https://github.com/apache/incubator-streampark/pull/3310#issuecomment-1793621290
Resolved~
--
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: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org