You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/11/12 13:59:43 UTC
[GitHub] [skywalking] wankai123 opened a new pull request, #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
wankai123 opened a new pull request, #9954:
URL: https://github.com/apache/skywalking/pull/9954
- [X] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #9953
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020775544
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,14 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // Do not check index settings in the "no-init mode",
+ // because the no-init mode OAP server doesn't take responsibility for index settings.
Review Comment:
Does no init mode take responsibility of index mappings?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020775885
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,14 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // Do not check index settings in the "no-init mode",
+ // because the no-init mode OAP server doesn't take responsibility for index settings.
Review Comment:
no-init mode needs check and wait init mappings
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020775885
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,14 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // Do not check index settings in the "no-init mode",
+ // because the no-init mode OAP server doesn't take responsibility for index settings.
Review Comment:
no-init mode needs check and wait init mappings, it's use `contains`, so would not break start procedure .
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#issuecomment-1312514510
The changes LGTM
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020775885
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,14 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // Do not check index settings in the "no-init mode",
+ // because the no-init mode OAP server doesn't take responsibility for index settings.
Review Comment:
no-init mode needs check and wait init mappings, it's use `contains`, so would not break start procedure, if no-init mode fields less than init mode. But if more than init-mode, it should break, because will miss fields create.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#issuecomment-1312508979
@wankai123 I didn't change it locally, so don't know whether it is 100% correct.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020774110
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,15 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ boolean compareIndexSetting = structures.compareIndexSetting(tableName, createSetting(model));
+ // "no-init mode" no needs to check index settings for updating,
+ // to avoid conflicts between "init mode and no-init mode" index settings configurations
+ if (RunningMode.isNoInitMode()) {
+ exist = containsMapping;
+ } else {
+ exist = containsMapping && compareIndexSetting;
+ }
Review Comment:
```suggestion
boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
// Do not check index settings in the "no-init mode",
// because the no-init mode OAP server doesn't take responsibility for index settings.
if (RunningMode.isNoInitMode()) {
exist = containsMapping;
} else {
exist = containsMapping && structures.compareIndexSetting(tableName, createSetting(model));
}
```
I think this should be better in perf.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020774553
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -104,8 +112,15 @@ public boolean isExists(Model model) throws StorageException {
structures.putStructure(
tableName, template.get().getMappings(), template.get().getSettings()
);
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ boolean compareIndexSetting = structures.compareIndexSetting(tableName, createSetting(model));
+ // "no-init mode" no needs to check index settings for updating,
+ // to avoid conflicts between "init mode and no-init mode" index settings configurations
+ if (RunningMode.isNoInitMode()) {
+ exist = containsMapping;
+ } else {
+ exist = containsMapping && compareIndexSetting;
+ }
Review Comment:
A similar suggestion as above.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9954:
URL: https://github.com/apache/skywalking/pull/9954
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020775531
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -104,8 +111,14 @@ public boolean isExists(Model model) throws StorageException {
structures.putStructure(
tableName, template.get().getMappings(), template.get().getSettings()
);
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // "no-init mode" no needs to check index settings for updating,
+ // to avoid conflicts between "init mode and no-init mode" index settings configurations
Review Comment:
```suggestion
// Do not check index settings in the "no-init mode",
// because the no-init mode OAP server doesn't take responsibility for index settings.
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #9954: Fix Elasticsearch storage installer: ignore check index settings for updating in no-init running mode
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9954:
URL: https://github.com/apache/skywalking/pull/9954#discussion_r1020776060
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -84,8 +85,14 @@ public boolean isExists(Model model) throws StorageException {
Optional<Index> index = esClient.getIndex(tableName);
Mappings historyMapping = index.map(Index::getMappings).orElseGet(Mappings::new);
structures.putStructure(tableName, historyMapping, index.map(Index::getSettings).orElseGet(HashMap::new));
- exist = structures.containsMapping(tableName, createMapping(model))
- && structures.compareIndexSetting(tableName, createSetting(model));
+ boolean containsMapping = structures.containsMapping(tableName, createMapping(model));
+ // Do not check index settings in the "no-init mode",
+ // because the no-init mode OAP server doesn't take responsibility for index settings.
Review Comment:
Kind of. As OAP should wait for the index mappings contain the required.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org