[Improve]Improve flink env check method #3461
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
…o improvement-flink-env-check # Conflicts: # streampark-common/src/main/java/org/apache/streampark/common/enums/FlinkEnvStatus.java
zzzk1
changed the title
|
Looks good, The front-end code also needs improvement. Looking forward to your next update! 😊 |
wolfboys
reviewed
Jan 7, 2024
| */ | ||
| Integer check(FlinkEnv version); | ||
| String check(FlinkEnv version); |
There was a problem hiding this comment.
I think it should be better to return type FlinkEnvStatus
wolfboys
reviewed
Jan 7, 2024
| FEASIBLE("ok"), | ||
|
|
||
| /* defined flink name repeated */ | ||
| NAME_REPEATED("name repeated"), |
There was a problem hiding this comment.
Why remove the code? Need an attribute of type Int(code)
wolfboys
reviewed
Jan 7, 2024
| @@ -52,7 +52,7 @@ public RestResponse list() { | |||
| @Operation(summary = "Verify flink environment") | |||
| @PostMapping("check") | |||
| public RestResponse check(FlinkEnv version) { | |||
| Integer checkResp = flinkEnvService.check(version); | |||
| String checkResp = flinkEnvService.check(version); | |||
| return RestResponse.success(checkResp); | |||
There was a problem hiding this comment.
Return the code value of the enum FlinkEnvStatus
wolfboys
reviewed
Jan 7, 2024
| const checkResp = parseInt(resp.data); | ||
| if (checkResp != 0) { | ||
| const checkResp = resp.data; | ||
| if (checkResp != 'ok') { |
There was a problem hiding this comment.
Here is how the front-end (FE) use enums to adapt to the values returned by the back-end (BE), You can refer to it:
zzzk1
force-pushed
the
improvement-flink-env-check
branch
from
27f13ff to
5b90bdf
Compare
wolfboys
reviewed
Jan 7, 2024
| @@ -49,8 +50,6 @@ public class FlinkEnvServiceImpl extends ServiceImpl<FlinkEnvMapper, FlinkEnv> | |||
| * two places will be checked: <br> | |||
| * 1) name repeated <br> | |||
| * 2) flink-dist repeated <br> | |||
| * -1) invalid path <br> | |||
| * 0) ok <br> | |||
| */ | |||
| @Override | |||
| public Integer check(FlinkEnv version) { | |||
There was a problem hiding this comment.
It should be better to return FlinkEnvStatus type instead of Integer
wolfboys
reviewed
Jan 7, 2024
streampark-console/streampark-console-webapp/src/views/flink/home/components/Modal.vue
Show resolved
Hide resolved
wolfboys
reviewed
Jan 7, 2024
| if (checkResp !== FlinkEvnEnum.FEASIBLE) { | ||
| switch (checkResp) { | ||
| case FlinkEvnEnum.INVALID: | ||
| Swal.fire('Failed', 'FLINK_HOME invalid path.', 'error'); |
| case FlinkEvnEnum.FLINK_DIST_REPEATED: | ||
| Swal.fire( | ||
| 'Failed', | ||
| 'can no found flink-dist or found multiple flink-dist, FLINK_HOME error.', |
There was a problem hiding this comment.
error msg should use i18n, e.g: t('setting.flinkHome.operateMessage.FLINK_DIST_REPEATED')
...
There was a problem hiding this comment.
sry I know nothing about the front end
zzzk1
changed the title
|
cc @RocMarshal |
wolfboys
reviewed
Jan 8, 2024
| @@ -32,6 +32,8 @@ export default { | |||
| flinkNameIsRequired: 'Flink名称必填', | |||
| flinkHomeTips: 'Flink所在服务器的绝对路径,举例: /usr/local/flink', | |||
| flinkHomeIsRequired: 'Flink安装路径必填', | |||
| flinkHomePathIsInvalid: 'Flink所在路径不合法', | |||
| flinkDistIsRepeated: '找不到发行版代码', | |||
There was a problem hiding this comment.
flink lib 路径下有且只能有一个 flink-dist jar 文件
| @@ -32,6 +32,8 @@ export default { | |||
| flinkNameIsRequired: 'Flink名称必填', | |||
| flinkHomeTips: 'Flink所在服务器的绝对路径,举例: /usr/local/flink', | |||
| flinkHomeIsRequired: 'Flink安装路径必填', | |||
| flinkHomePathIsInvalid: 'Flink所在路径不合法', | |||
wolfboys
approved these changes
Jan 8, 2024
zzzk1
added a commit
to zzzk1/incubator-streampark
that referenced
this pull request
Jan 12, 2024
* [Improve] improve hard code in FlinkEnvServiceImpl * improve front end hard code and back end check method * improve check method return type * improve front end check method logic * improve message support i18n * improve i18n error message --------- Co-authored-by: benjobs <[email protected]>
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.
What changes were proposed in this pull request
improve hard code in FlinkEnvServiceImpl check method
Brief change log
add FlinkEnvStatus.java in
streampark-common/src/main/java/org/apache/streampark/common/enums/
modifty FlinkEnvServiceImpl.java in
streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/FlinkEnvServiceImpl.java
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts