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 2021/09/23 01:10:29 UTC
[GitHub] [skywalking] wu-sheng opened a new pull request #7772: Only report `bug warning` in precise conditions.
wu-sheng opened a new pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
User will see the `[Bug warning]` if the template exists and index deleted, or in reverse. In this case, this isn't the bug report we expected.
--
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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714425557
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
> The supported versions are fixed and the codes won't run on un-tested versions.
I know we have `{"6.3.2"}, {"7.4.2"}, {"7.8.0"}, {"7.10.2"}`, but who knows one day, something breaks unexpected, such as we never knew in the `7.x` there are different cases.
> It's good to me to keep the new condition, but please also add a test to
Added check for template existing 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: 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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714425557
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
> The supported versions are fixed and the codes won't run on un-tested versions.
I know we have `{"6.3.2"}, {"7.4.2"}, {"7.8.0"}, {"7.10.2"}`, but who knows one day, something breaks unexpected, such as we never would know in the `7.x` there are different cases.
> It's good to me to keep the new condition, but please also add a test to
Added check for template existing 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714419315
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
If this is the only condition, I think a basic test should cover it, this is a very basic logic and should be discovered at test phase, not runtime
```java
assertThat(templateClient.exists(name)).isTrue();
```
https://github.com/apache/skywalking/blob/73b6c67344d83cabe3014fc33898eb15f3bc4063/oap-server/server-library/library-elasticsearch-client/src/test/java/org/apache/skywalking/library/elasticsearch/ITElasticSearchTest.java#L102-L107
--
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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714422961
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
It's good to me to keep the new condition, but please also add a test to https://github.com/apache/skywalking/blob/73b6c67344d83cabe3014fc33898eb15f3bc4063/oap-server/server-library/library-elasticsearch-client/src/test/java/org/apache/skywalking/library/elasticsearch/ITElasticSearchTest.java#L102-L107
```java
assertThat(templateClient.exists(name)).isTrue();
```
--
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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714419619
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
I was proposing this due to we can't test all server versions, and as we are not official, we can't tell when breaking change happens.
This exception codes should never be executed.
--
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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714419866
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
The current conditions checking could fail in daily used, it is not a bug, and we have to suffer endless question and bug report on GitHub.
--
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 #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772
--
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 change in pull request #7772: Only report `bug warning` in precise conditions.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7772:
URL: https://github.com/apache/skywalking/pull/7772#discussion_r714422617
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -70,14 +70,17 @@ protected boolean isExists(Model model) {
if (!model.isTimeSeries()) {
return esClient.isExistsIndex(tableName);
}
- boolean exist = esClient.isExistsTemplate(tableName)
- && esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
+ boolean templateExists = esClient.isExistsTemplate(tableName);
final Optional<IndexTemplate> template = esClient.getTemplate(tableName);
+ boolean lastIndexExists = esClient.isExistsIndex(TimeSeriesUtils.latestWriteIndexName(model));
- if ((exist && !template.isPresent()) || (!exist && template.isPresent())) {
- throw new Error("[Bug warning]ElasticSearch client query template result is not consistent. Please file an issue to Apache SkyWalking.(https://github.com/apache/skywalking/issues)");
+ if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent())) {
Review comment:
> The current conditions checking could fail in daily used, it is not a bug, and we have to suffer endless question and bug report on GitHub.
I don't mean to keep the current conditions, I mean if the condition is to be `if ((templateExists && !template.isPresent()) || (!templateExists && template.isPresent()))`, then this is a very basic function that we should test in the perspective of ES client.
> I was proposing this due to we can't test all server versions, and as we are not official, we can't tell when breaking change happens.
The supported versions are fixed and the codes won't run on un-tested versions.
https://github.com/apache/skywalking/blob/73b6c67344d83cabe3014fc33898eb15f3bc4063/oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/ElasticSearchVersion.java#L67
We will manually test a new ES version, and add it into our supported list, otherwise if users run on a future ES server for example 8.2, the OAP won't even start
--
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