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

fix: allow timer to restart #112

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

shreemaan-abhishek
Copy link
Contributor

The APISIX skywalking plugin reload creates a scenario where destroyBackendTimer() is called:

https://github.com/apache/apisix/blob/a478de8a586671bc7dd0dcf89d8db18d9913ab7c/apisix/plugins/skywalking.lua#L154

but after successful reload. When startBackendTimer() is called:

https://github.com/apache/apisix/blob/a478de8a586671bc7dd0dcf89d8db18d9913ab7c/apisix/plugins/skywalking.lua#L144

it won't succeed because self.stopped still remains true.

This issue has been fixed.

@wu-sheng
Copy link
Member

This fix should be fine. But I want to confirm what is the case that restarts the timer?

@wu-sheng wu-sheng added this to the 1.1.0 milestone Sep 24, 2024
@wu-sheng wu-sheng added the bug Something isn't working label Sep 24, 2024
@wu-sheng wu-sheng merged commit 56e09ec into apache:master Sep 24, 2024
2 checks passed
@shreemaan-abhishek shreemaan-abhishek deleted the fix-client-restart branch September 24, 2024 12:39
@shreemaan-abhishek
Copy link
Contributor Author

But I want to confirm what is the case that restarts the timer?

During skywalking plugin reload in APISIX, timer restart case can be observed.

@shreemaan-abhishek
Copy link
Contributor Author

When will this new version v1.1.0 be released?

@wu-sheng
Copy link
Member

I don't have plan to do so. Maybe you could ask people from api7? You have committer to initial patch release(1.0.1) if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants