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: add tls common conf hibernate after #9967

Merged

Conversation

qzhuyan
Copy link
Contributor

@qzhuyan qzhuyan commented Feb 13, 2023

Fixes

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/<version>/(feat|fix)-<PR-id>.en.md and .zh.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

@qzhuyan qzhuyan force-pushed the perf/william/add-tls-common-conf-hibernate-after branch from a392be2 to b3789aa Compare February 14, 2023 07:50
@qzhuyan
Copy link
Contributor Author

qzhuyan commented Feb 14, 2023

do you think we should intro a value disabled or undefined to turn it off or just use some semi xlarge value like 1000 hours?

currently having issues letting hocon and emqx_config cope with the value undefined.
And disabled need some extra convert work here and there that leads to extra overhead on maintaining the code since this opt is common opt for all TLS servers and clients.

sc(
duration(),
#{
default => Df("hibernate_after", "5s"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here follows emqx 4.x that has default 5s and personally I think it will be much easier to make it default to be disabled undefined

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd recommend using infinity instead of undefined for this.
Although their effect is the same.
But our other configurations are using infinity
such as

 {"idle_timeout",
            sc(
                hoconsc:union([infinity, duration()])
                ...

https://www.erlang.org/doc/man/erlang.html#type-timeout

hibernate_after() = timeout()
timeout() = infinity | integer() >= 0

By the way, what happens if we set it to 0, is 0 the same as infinity?
Maybe we should also mention this in the document.

@qzhuyan qzhuyan marked this pull request as ready for review February 14, 2023 09:10
@qzhuyan qzhuyan requested review from a team and lafirest as code owners February 14, 2023 09:10
}
label: {
en: "hibernate after"
zh: "闲置后休眠时间"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 多久后休眠 will be better? Sorry, I can't think of a better word

@qzhuyan qzhuyan force-pushed the perf/william/add-tls-common-conf-hibernate-after branch from b3789aa to 4e6c7bf Compare February 14, 2023 15:27
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4175359685

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 42 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-0.02%) to 80.838%

Files with Coverage Reduction New Missed Lines %
apps/emqx_dashboard/src/emqx_dashboard_monitor.erl 1 69.44%
apps/emqx_gateway/src/mqttsn/emqx_sn_channel.erl 1 73.04%
apps/emqx/src/emqx_cm.erl 1 90.83%
apps/emqx/src/emqx_flapping.erl 1 85.11%
apps/emqx/src/emqx_session.erl 1 87.6%
apps/emqx_management/src/emqx_mgmt_api_trace.erl 2 86.67%
apps/emqx/src/emqx_channel.erl 2 87.58%
apps/emqx_resource/src/emqx_resource_buffer_worker.erl 3 93.04%
apps/emqx/src/emqx_broker.erl 4 83.15%
apps/emqx_management/src/emqx_mgmt_cli.erl 5 27.43%
Totals Coverage Status
Change from base Build 4173661439: -0.02%
Covered Lines: 23734
Relevant Lines: 29360

💛 - Coveralls

@qzhuyan qzhuyan merged commit 02f15be into emqx:master Feb 15, 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

5 participants