-
Notifications
You must be signed in to change notification settings - Fork 538
integration-server-test: Don't notify if not run from main #17209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
endorama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
Just noted your test run failed. Tests that failed where:
Both errors don't seem related to this change. Worth retrying them before merging. |
|
Made some last minute changes, will rerun the test again. |
|
fyi the change in elastic/elasticsearch#128913 is going to be backported to 8.19. This means you should expect a change in template between "8.18 and 8.19" and "9.0 and 9.1" |
|
Thanks for the heads up. I will revert the changes then. |
This reverts commit aeddc56.
| }) | ||
|
|
||
| scrollSearchQueryContextCanceled = apmErrorLog(types.Query{ | ||
| MatchPhrase: map[string]types.MatchPhraseQuery{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we think of excluding all "context canceled" logs that we observe? I expect there are multiple occasions where this happens on shutdown and filtering them one by one manually seems to require a lot of manual intervention. This is for a follow-up, worth a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, "context canceled" (and maybe even "context deadline exceeded") could be excluded in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericywl I would have preferred if you closed this PR instead of renaming it with a complete different scope, this may create some confusion (think already sent notifications/emails).
Said that let's merge this, the change to the notification is great!
I'd need you to clarify if with the last comment from Carson you reverted the changes because not relevant or because you plan to introduce them in another PR. My understanding is that with the template change there would be a rollover in 8.19, but maybe this already happens so it's already covered?
Got it, will take not for future PRs.
Once the |
Motivation/summary
Due to changes toapm-dataintroduced in elastic/elasticsearch#128913, 8.19 and 9.1 no longer share the same templates. As a result, when upgrading from 8.19 to 9.1, there will be index lazy-rollover on ingest.This PR fixes the failure caused by the above, and also update notify step to fire on main branch only.It seems the failures is simply due to the
apm-datachange not being backported to 8.19 yet. I have reverted the relevant changes here, but left the PR up since there are some good changes.