Skip to content
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

[Heartbeat] Fix Continuation Dispatch / mode: all pings #12458

Merged
merged 3 commits into from
Jun 7, 2019

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Jun 6, 2019

Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is mode:all pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This patch also renames some of the related variables to make understanding the code here simpler.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes

Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is `mode:all` pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes
@andrewvc andrewvc added bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.2.0 labels Jun 6, 2019
@andrewvc andrewvc requested a review from ruflin June 6, 2019 02:51
@andrewvc andrewvc requested a review from a team as a code owner June 6, 2019 02:51
@andrewvc andrewvc self-assigned this Jun 6, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Code change LGTM. When did we introduce this or was it always broken?

Could you add a changelog entry?

@ruflin ruflin added the needs_backport PR is waiting to be backported to other branches. label Jun 6, 2019
@andrewvc
Copy link
Contributor Author

andrewvc commented Jun 6, 2019

@ruflin looks like I introduced this in 7.0.1 when I inlined this function call https://github.com/elastic/beats/pull/11895/files#diff-4d4178828d8a492b11ba6f28898cef10L122

@@ -162,15 +162,20 @@ func runPublishJob(job jobs.Job, client beat.Client) []scheduler.TaskFunc {
}
}

if len(next) == 0 {
if !hasContinuations {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this here since it makes the code more readable. I know this is a bug fix, but I think some small cleanup of this function goes to that since hard to maintain code carries bug risk.

@andrewvc
Copy link
Contributor Author

andrewvc commented Jun 6, 2019

@ruflin added the CHANGELOG entry, also cleaned up one small thing. LMK if you think that's out of scope.

@andrewvc andrewvc merged commit e491e9a into elastic:master Jun 7, 2019
@andrewvc andrewvc deleted the fix-mode-all branch June 7, 2019 13:49
andrewvc added a commit to andrewvc/beats that referenced this pull request Jun 7, 2019
Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is `mode:all` pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes

(cherry picked from commit e491e9a)
@andrewvc andrewvc removed the needs_backport PR is waiting to be backported to other branches. label Jun 7, 2019
andrewvc added a commit that referenced this pull request Jun 7, 2019
)

Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is `mode:all` pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes

(cherry picked from commit e491e9a)
andrewvc added a commit to andrewvc/beats that referenced this pull request Jun 12, 2019
Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is `mode:all` pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
… (elastic#12477)

Currently mode: all pings are broken. The root cause is that the scheduler dispatch of continuations in the heartbeat task model is broken. Since the only current use of this is `mode:all` pings that is what is affected. The issue was incorrectly aliasing variables when dispatching future work.

This issue is well described on this wiki page https://github.com/golang/go/wiki/CommonMistakes

(cherry picked from commit 4cc0a1c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants