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({kafka,pulsar}_producer): correctly handle metrics for connectors that have internal buffers #11724

Merged
merged 5 commits into from Oct 10, 2023

Conversation

thalesmg
Copy link
Contributor

@thalesmg thalesmg commented Oct 6, 2023

targeting release-53

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

There’s currently a metric inconsistency due to the internal buffering nature of Kafka Producer (wolff).

We use simple_sync_query to call the Kafka Producer bridge. If that times out, the call is accounted as failed, even though the message is buffered in wolff and later sent successfully.

Summary

🤖 Generated by Copilot at 887f0ad

This pull request adds a new query mode simple_sync_internal_buffer to the emqx_resource application, which enables synchronous queries with internal buffering and retry logic for resource workers that support it. This mode is used by the Kafka bridge producer to improve message delivery reliability. The pull request also updates the type definitions, version number, and query function of the emqx_resource application.

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
  • Added property-based tests for code which performs user input validation
  • 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
  • Created PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket
  • 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 fix-kprodu-sync-metrics-m-20231006 branch 2 times, most recently from f991615 to 52bc762 Compare October 6, 2023 20:45
@thalesmg thalesmg changed the title fix(kafka_producer): correctly handle metrics for connector that have internal buffers fix(kafka_producer): correctly handle metrics for connectors that have internal buffers Oct 6, 2023
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6436313820

  • 13 of 19 (68.42%) changed or added relevant lines in 2 files are covered.
  • 15 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.006%) to 82.736%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/emqx_resource/src/emqx_resource_buffer_worker.erl 12 18 66.67%
Files with Coverage Reduction New Missed Lines %
apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub_consumer_worker.erl 1 93.47%
apps/emqx_ft/src/emqx_ft_assembly.erl 1 97.12%
apps/emqx_resource/src/emqx_resource_buffer_worker.erl 1 92.75%
apps/emqx/src/bpapi/emqx_bpapi.erl 1 95.83%
apps/emqx/src/emqx_broker.erl 1 88.52%
apps/emqx/src/emqx_connection.erl 1 83.15%
apps/emqx/src/emqx_reason_codes.erl 9 91.91%
Totals Coverage Status
Change from base Build 6435238318: 0.006%
Covered Lines: 33800
Relevant Lines: 40853

💛 - Coveralls

@thalesmg thalesmg marked this pull request as ready for review October 9, 2023 12:05
@thalesmg thalesmg requested a review from a team as a code owner October 9, 2023 12:05
@thalesmg thalesmg changed the title fix(kafka_producer): correctly handle metrics for connectors that have internal buffers fix({kafka,pulsar}_producer): correctly handle metrics for connectors that have internal buffers Oct 9, 2023
@thalesmg thalesmg force-pushed the fix-kprodu-sync-metrics-m-20231006 branch 4 times, most recently from a1c17c0 to 7c690a1 Compare October 9, 2023 14:23
… internal buffers

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

There’s currently a metric inconsistency due to the internal buffering nature of Kafka
Producer (wolff).

We use simple_sync_query to call the Kafka Producer bridge.  If that times out, the call
is accounted as failed, even though the message is buffered in wolff and later sent
successfully.
…ridges with internal buffering

Since authn/authz backends also use simple async/sync queries, we may want to avoid them
calling the connector when it's not connected.
…or Pulsar

Since it has internal buffering, it necessitates the same fix as Kafka producer.
@thalesmg thalesmg force-pushed the fix-kprodu-sync-metrics-m-20231006 branch from c3bd534 to d6781ef Compare October 9, 2023 18:02
@thalesmg thalesmg changed the base branch from master to release-53 October 9, 2023 18:05
@thalesmg thalesmg closed this Oct 9, 2023
@thalesmg thalesmg reopened this Oct 9, 2023
@zmstone
Copy link
Member

zmstone commented Oct 10, 2023

there is no need to keep the is_buffer_suppored option in apps/emqx_resource/src/emqx_resource.erl

@thalesmg
Copy link
Contributor Author

there is no need to keep the is_buffer_suppored option in apps/emqx_resource/src/emqx_resource.erl

It was removed here: https://github.com/emqx/emqx/pull/11722/files#diff-3803c5a6f7df225ee8e37ccda7a79158464e838d842bd5632f25e6356fffb7c2L309

But there's still a reference to it in emqx_resource.hrl. I'll remove it.

@thalesmg thalesmg merged commit ee45145 into emqx:release-53 Oct 10, 2023
142 checks passed
@thalesmg thalesmg deleted the fix-kprodu-sync-metrics-m-20231006 branch October 10, 2023 15:13
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