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

feat(bridges): enable async query mode for all bridges with buffer workers #10306

Merged

Conversation

thalesmg
Copy link
Contributor

@thalesmg thalesmg commented Mar 31, 2023

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

Since buffer workers always support async calls ("outer calls"), we should decouple those two call modes (inner and outer), and avoid exposing the inner call configuration to user to avoid complexity.

For bridges that currently only allow sync query modes, we should allow them to be configured with async. That means basically all bridge types except Kafka Producer.

Summary

🤖 Generated by Copilot at 066f73f

This pull request simplifies and unifies the schema definitions for various bridge resources by reusing the common resource_opts field from the emqx_resource_schema module and removing unused or redundant fields. It also makes the query_mode field more flexible and consistent across different resources, allowing both sync and async modes.

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

@thalesmg thalesmg force-pushed the enable-async-buffer-workers-all-bridges-v50 branch 7 times, most recently from 02a4470 to 17f10ab Compare April 3, 2023 15:02
@coveralls
Copy link
Collaborator

coveralls commented Apr 3, 2023

Pull Request Test Coverage Report for Build 4599750578

  • 14 of 14 (100.0%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 81.048%

Totals Coverage Status
Change from base Build 4598063119: -0.008%
Covered Lines: 25035
Relevant Lines: 30889

💛 - Coveralls

…rkers

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

Since buffer workers always support async calls ("outer calls"), we
should decouple those two call modes (inner and outer), and avoid
exposing the inner call configuration to user to avoid complexity.

For bridges that currently only allow sync query modes, we should
allow them to be configured with async.  That means basically all
bridge types except Kafka Producer.
@thalesmg thalesmg force-pushed the enable-async-buffer-workers-all-bridges-v50 branch from 17f10ab to f3ffc02 Compare April 3, 2023 17:49
@thalesmg
Copy link
Contributor Author

thalesmg commented Apr 3, 2023

Noticed an issue with rule action statistics when using async actions. They always count as successes, because the rule engine doesn't wait for the reply. The bridge/connector returns just ok when async (it's just a cast), and that's immediately interpreted as a success.

Bridge (don't mind the 6 total; 2 messages were sent before restarting the rule stats):
image

Rule:
image

@thalesmg
Copy link
Contributor Author

thalesmg commented Apr 3, 2023

Noticed an issue with rule action statistics when using async actions. They always count as successes, because the rule engine doesn't wait for the reply. The bridge/connector returns just ok when async (it's just a cast), and that's immediately interpreted as a success.

Actually, this issue exists prior to the present changes. So we could merge this one as it is and create a follow up issue for fixing those metrics.

@thalesmg thalesmg marked this pull request as ready for review April 3, 2023 19:41
@thalesmg thalesmg requested review from a team, kjellwinblad and JimMoen as code owners April 3, 2023 19:41
@zmstone
Copy link
Member

zmstone commented Apr 4, 2023

Noticed an issue with rule action statistics when using async actions. They always count as successes, because the rule engine doesn't wait for the reply. The bridge/connector returns just ok when async (it's just a cast), and that's immediately interpreted as a success.

Actually, this issue exists prior to the present changes. So we could merge this one as it is and create a follow up issue for fixing those metrics.

yeah, it's a known "issue", and some argue it's not an issue.

@thalesmg thalesmg merged commit 5d5b7ea into emqx:master Apr 4, 2023
100 of 101 checks passed
@thalesmg thalesmg deleted the enable-async-buffer-workers-all-bridges-v50 branch April 4, 2023 20:10
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