Skip to content

Only report bug warning in precise conditions.#7772

Merged
wu-sheng merged 2 commits into
masterfrom
condition-precise
Sep 23, 2021
Merged

Only report bug warning in precise conditions.#7772
wu-sheng merged 2 commits into
masterfrom
condition-precise

Conversation

@wu-sheng
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng commented Sep 23, 2021

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

User will see the [Bug warning] in 2 typical cases

  1. If the template exists and index deleted, or in reverse. In this case, this isn't the bug report we expected.
  2. OAP shut down over a day and latest index doesn't exist for sure. In this case, we just need to return false, and create method will fix this.

@wu-sheng wu-sheng added the enhancement Enhancement on performance or codes label Sep 23, 2021
@wu-sheng wu-sheng added this to the 8.8.0 milestone Sep 23, 2021
@wu-sheng wu-sheng requested a review from kezhenxu94 September 23, 2021 01:10

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())) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

        assertThat(templateClient.exists(name)).isTrue();

assertThat(templateClient.get(name))
.isPresent()
.map(IndexTemplate::getMappings)
.map(Mappings::getProperties)
.hasValue(mappings.getProperties());
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to me to keep the new condition, but please also add a test to

assertThat(templateClient.get(name))
.isPresent()
.map(IndexTemplate::getMappings)
.map(Mappings::getProperties)
.hasValue(mappings.getProperties());
}

        assertThat(templateClient.exists(name)).isTrue();

Copy link
Copy Markdown
Member Author

@wu-sheng wu-sheng Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wu-sheng wu-sheng requested a review from kezhenxu94 September 23, 2021 02:05
@wu-sheng wu-sheng merged commit c4b8c66 into master Sep 23, 2021
@wu-sheng wu-sheng deleted the condition-precise branch September 23, 2021 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement on performance or codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants