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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

0517 fix bridge update timeout issue #10755

Merged
merged 7 commits into from May 20, 2023

Conversation

zmstone
Copy link
Member

@zmstone zmstone commented May 19, 2023

Fixes https://emqx.atlassian.net/browse/EMQX-9873

fix(resource-manager): ensure no false creation

Update is implemented as remove + create.
If a dleete call is made while the create is in progress
the remove call is likely to timeout too.

This causes the follwing creation to falsely succeed,
because there is alreay a running child under the supervisor.

As a result, the resource is permanently removed after
resource_manager eventually handles the remove call.

Summary

馃 Generated by Copilot at 53ffa28

This pull request improves the emqx_connector and emqx_resource applications by enhancing the performance, resource usage, error handling, and logging of the connector and the resource manager. It also fixes a bug in the dev script and adds a new feature of dynamic resource creation and deletion based on the connector configuration. It updates the version number and the changelog accordingly.

PR Checklist

Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:

  • Added tests for the changes
  • Changed lines covered in coverage report
  • Change log has been added to changes/{ce,ee}/(feat|perf|fix)-<PR-id>.en.md files
  • For internal contributor: there is a jira ticket to track this change
  • [~] If there should be document changes, a PR to emqx-docs.git is sent, or a jira ticket is created to follow up
  • [~] Schema changes are backward compatible

Checklist for CI (.github/workflows) changes

  • [~] If changed package build workflow, pass this action (manual trigger)
  • [~] Change log has been added to changes/ dir for user-facing artifacts update

@zmstone zmstone force-pushed the 0517-fix-bridge-update-timeout-issue branch 3 times, most recently from 53ffa28 to ad8550e Compare May 19, 2023 16:35
ieQu1
ieQu1 previously approved these changes May 19, 2023
connect failure should be at warning level but not error,
the connecting state is visiable from dashbaord
also the resource manager logs connection failures in general
at warning level
the timeout shutdown in child spec may
significantly slow down the deletion of a resource

this commit chagnes the shutdown to brutal kill

also, the pool worker removal code has been delete
because it's not necessary since the entier pool is
going to be force-delete later anyway
the shutdown timeout is now set to infinity so it will never
force kill a resource manager, otherwise there will be
resource leaks
Update is implemented as remove + create.
If a dleete call is made while the create is in progress
the remove call is likely to timeout too.

This causes the follwing creation to falsely succeed,
because there is alreay a running child under the supervisor.

As a result, the resource is permanently removed after
resource_manager eventually handles the remove call.
@zmstone zmstone force-pushed the 0517-fix-bridge-update-timeout-issue branch from ad8550e to f4aa780 Compare May 19, 2023 16:55
thalesmg
thalesmg previously approved these changes May 19, 2023
changes/ce/fix-10755.en.md Outdated Show resolved Hide resolved
@zmstone zmstone marked this pull request as ready for review May 19, 2023 18:46
@zmstone zmstone requested review from a team and JimMoen as code owners May 19, 2023 18:46
@zmstone zmstone merged commit 3e98b3b into emqx:release-50 May 20, 2023
146 checks passed
@zmstone zmstone deleted the 0517-fix-bridge-update-timeout-issue branch May 20, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants