You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@streampark.apache.org by GitBox <gi...@apache.org> on 2022/09/20 16:10:07 UTC
[GitHub] [incubator-streampark] youyangkou opened a new pull request, #1659: [Bug]Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
youyangkou opened a new pull request, #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659
#### fix the flink on k8s application build failed and savepointPath save failed bug when use h2 as database
<!--
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.
### What problem does this PR solve?
Issue Number: close #xxx <!-- REMOVE this line if no issue to close -->
Problem Summary:
1. because the initSetting has @PostConstruct in SettingServiceImpl class ,the variable settings size is 0 when streampark console start.
so when update setting the code ```settings.get(setting.getSettingKey()).setSettingValue(value)``` will case Nullpoint Exception and the variable settings'size will be 0 Always.
![Uploading image.png…]()
2. the table t_flink_savepoint in H2 missing a column 'chk_id'。
### What is changed and how it works?
Proposal: [xxx](url) <!-- REMOVE this line if not applicable -->
Feel free to ping committers for the review!
## Contribution Checklist
- Make sure that the pull request corresponds to a [GITHUB issue](https://github.com/apache/streampark/issues).
- Name the pull request in the form "[Feature] [component] Title of the pull request", where *Feature* can be replaced by `Hotfix`, `Bug`, etc.
- Minor fixes should be named following this pattern: `[hotfix] [docs] Fix typo in README.md doc`.
## Purpose of this pull request
<!-- Describe the purpose of this pull request. For example: This pull request adds checkstyle plugin.-->
--
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
[GitHub] [incubator-streampark] youyangkou commented on pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#issuecomment-1253096442
@wolfboys PTAL
--
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
[GitHub] [incubator-streampark] youyangkou commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975952785
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -81,6 +74,9 @@ public boolean update(Setting setting) {
if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
+ if (settings == null || settings.size() == 0) {
Review Comment:
> Is it necessary to init settings again?
It is necessary, the settings are empty when the data has not been loaded in the H2 database
--
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
[GitHub] [incubator-streampark] lvshaokang commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
lvshaokang commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975618701
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -81,6 +74,9 @@ public boolean update(Setting setting) {
if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
+ if (settings == null || settings.size() == 0) {
Review Comment:
Is it necessary to init settings again?
##########
streampark-console/streampark-console-service/src/main/resources/db/schema-h2.sql:
##########
@@ -197,6 +197,7 @@ create table if not exists `t_flink_project` (
create table if not exists `t_flink_savepoint` (
`id` bigint generated by default as identity not null,
`app_id` bigint not null,
+ `chk_id` bigint default null,
Review Comment:
Plz check pgsql and mysql dialect whether lack field or without default value.
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -81,6 +74,9 @@ public boolean update(Setting setting) {
if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
+ if (settings == null || settings.size() == 0) {
+ initSetting();
+ }
settings.get(setting.getSettingKey()).setSettingValue(value);
Review Comment:
if not, suggest use optional to resolve npe. btw, settings is not null, refer to 51 line.
```suggestion
Optional<Setting> optional = Optional.ofNullable(settings.get(setting.getSettingKey()));
if (optional.isPresent()) {
optional.get().setSettingValue(value);
}
```
--
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
[GitHub] [incubator-streampark] wolfboys commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
wolfboys commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975974173
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -61,27 +62,27 @@ public void initSetting() {
@Override
public boolean update(Setting setting) {
try {
- String value = setting.getSettingValue();
- if (value != null) {
- if (StringUtils.isEmpty(value.trim())) {
- value = null;
- } else {
- value = setting.getSettingValue().trim();
- }
- }
+ String value = StringUtils.trimToNull(setting.getSettingValue());
setting.setSettingValue(value);
this.baseMapper.updateByKey(setting);
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_REMOTE_URL().key())) {
+ String settingKey = setting.getSettingKey();
+ if (CommonConfig.MAVEN_REMOTE_URL().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_REMOTE_URL(), value);
}
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_USER().key())) {
+ if (CommonConfig.MAVEN_AUTH_USER().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_USER(), value);
}
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
+ if (CommonConfig.MAVEN_AUTH_PASSWORD().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
- settings.get(setting.getSettingKey()).setSettingValue(value);
+ if (settings == null || settings.size() == 0) {
+ initSetting();
Review Comment:
This logic is not as expected. H2, MySQL, and pgsql should be consistent. Data in data-h2.sql will be inserted into `t_ setting` table. These are completed in the system initialization phase, initSetting will only run once, So `initSetting` method should not be called again
--
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
[GitHub] [incubator-streampark] wolfboys merged pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
wolfboys merged PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659
--
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
[GitHub] [incubator-streampark] youyangkou commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975999890
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -61,27 +62,27 @@ public void initSetting() {
@Override
public boolean update(Setting setting) {
try {
- String value = setting.getSettingValue();
- if (value != null) {
- if (StringUtils.isEmpty(value.trim())) {
- value = null;
- } else {
- value = setting.getSettingValue().trim();
- }
- }
+ String value = StringUtils.trimToNull(setting.getSettingValue());
setting.setSettingValue(value);
this.baseMapper.updateByKey(setting);
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_REMOTE_URL().key())) {
+ String settingKey = setting.getSettingKey();
+ if (CommonConfig.MAVEN_REMOTE_URL().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_REMOTE_URL(), value);
}
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_USER().key())) {
+ if (CommonConfig.MAVEN_AUTH_USER().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_USER(), value);
}
- if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
+ if (CommonConfig.MAVEN_AUTH_PASSWORD().key().equals(settingKey)) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
- settings.get(setting.getSettingKey()).setSettingValue(value);
+ if (settings == null || settings.size() == 0) {
+ initSetting();
Review Comment:
Thanks for your review, addressed.
--
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
[GitHub] [incubator-streampark] youyangkou commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975940244
##########
streampark-console/streampark-console-service/src/main/resources/db/schema-h2.sql:
##########
@@ -197,6 +197,7 @@ create table if not exists `t_flink_project` (
create table if not exists `t_flink_savepoint` (
`id` bigint generated by default as identity not null,
`app_id` bigint not null,
+ `chk_id` bigint default null,
Review Comment:
> Plz check pgsql and mysql dialect whether lack field or without default value.
mysql and pgsql have chk_id，this is data-h2.sql bug.
--
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
[GitHub] [incubator-streampark] youyangkou commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975952156
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -81,6 +74,9 @@ public boolean update(Setting setting) {
if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
+ if (settings == null || settings.size() == 0) {
+ initSetting();
+ }
settings.get(setting.getSettingKey()).setSettingValue(value);
Review Comment:
@lvshaokang Thanks for your review, addressed.
--
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
[GitHub] [incubator-streampark] youyangkou commented on a diff in pull request #1659: [Bug] Fix the flink on k8s application build failed and savepointPath saved failed bug when use h2 as database
Posted by GitBox <gi...@apache.org>.
youyangkou commented on code in PR #1659:
URL: https://github.com/apache/incubator-streampark/pull/1659#discussion_r975953677
##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/SettingServiceImpl.java:
##########
@@ -81,6 +74,9 @@ public boolean update(Setting setting) {
if (setting.getSettingKey().equals(CommonConfig.MAVEN_AUTH_PASSWORD().key())) {
InternalConfigHolder.set(CommonConfig.MAVEN_AUTH_PASSWORD(), value);
}
+ if (settings == null || settings.size() == 0) {
Review Comment:
> Is it necessary to init settings again?
You can test it in H2 database schema
--
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