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

perf(config): read zone conf once #10790

Merged
merged 13 commits into from Jun 1, 2023

Conversation

qzhuyan
Copy link
Contributor

@qzhuyan qzhuyan commented May 23, 2023

Fixes EMQX-9801

Summary

Prior to this change, the default zone could be referenced by the listeners and it is not necessarily an item in the CONF PT if it is not defined under root zones by user .
Reading config for the zone must be done through the emqx_config:get_zone_conf/2 in which it first reads if the default zone exists in the config, otherwise it fallback to reading from the root of EMQX as the default fallback. If the default zone does exist but the field is undefined, it will fallback to read from default as well.

Generally, it has the following issues:

  1. Inconsistent return val of emqx_config:get_zone_conf(default,[]) and emqx_config:get([zones, default]) that one of them return some value another read failure.

  2. Always read the config twice if no user-defined default zone or the field is undefined. (first read miss then fallback to default) which hurts overall performance if read is done in the hot code path.

  3. Read a zone with a none existing zone name (not default) will also return values that may confuse the users.

In this PR:

  1. In CONF, The default zone which is referenced by the listeners is now explicitly inserted into the runtime config during emqx config initialization during emqx schema loading. the values are merged with zone global defaults and user-defined values.

  2. in RAWCONF, it remains unchanged and it keeps a copy of user-defined configs for reference

  3. When there is an update on the global defaults, it will be merged into the zones.

  4. When there is an update on the existing zone, it is just a regular update no merge with global defaults.

  5. When there is an insertion of new zones, they will be merged with zone global defaults

  6. During initialization, loading config order is enforced. load zones after the other ROOTs.

Breaking changes:

  1. Read the none-existing zone will fail now.
  2. After emqx_schema is loaded, CONF PT stores the runtime config that has a default zone with complete children fields all set
  3. CONF is now the runtime config for hot code path reading while RAWCONF has user-defined values and they are not in sync for root zones (<<"zones">>)

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

@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch 9 times, most recently from 593590d to 4e753ce Compare May 26, 2023 11:22
@qzhuyan qzhuyan changed the base branch from master to release-51 May 26, 2023 12:42
@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch 5 times, most recently from 47177a7 to ce63b7f Compare May 30, 2023 06:28
@qzhuyan
Copy link
Contributor Author

qzhuyan commented May 30, 2023

note, with many different attempts in the PR, need to squash the commits before the merge.

@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch from 16c03f9 to 5c3707d Compare May 30, 2023 13:36
@qzhuyan qzhuyan marked this pull request as ready for review May 30, 2023 15:55
@qzhuyan qzhuyan requested review from a team, lafirest and sstrigler as code owners May 30, 2023 15:55
@qzhuyan qzhuyan changed the title WIP: perf(config): read zone conf once perf(config): read zone conf once May 30, 2023
@qzhuyan
Copy link
Contributor Author

qzhuyan commented May 31, 2023

also improve: emqx_mgmt_caps:get_caps

@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch from 85973d3 to 2148dde Compare May 31, 2023 09:51
@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch from 3d47689 to 9f964a1 Compare June 1, 2023 08:35
@qzhuyan
Copy link
Contributor Author

qzhuyan commented Jun 1, 2023

need squash after review

Copy link
Member

@zhongwencool zhongwencool left a comment

Choose a reason for hiding this comment

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

LGTM

@qzhuyan qzhuyan force-pushed the perf/william/explicit-default-zone branch from 9f964a1 to eea0336 Compare June 1, 2023 12:58
@qzhuyan qzhuyan merged commit 34958ce into emqx:release-51 Jun 1, 2023
139 of 141 checks passed
@yanzhiemq
Copy link
Collaborator

yanzhiemq commented Jun 13, 2023

Enhancements

  • Reduced the overhead during configuration reads by optimizing the configuration read mechanism.

@yanzhiemq
Copy link
Collaborator

yanzhiemq commented Jun 13, 2023

增强

  • 通过优化配置读取机制减少读取配置的开销。

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

4 participants