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

init and init_worker phases called on all policies #770

Merged
merged 8 commits into from
Jun 19, 2018

Conversation

mikz
Copy link
Contributor

@mikz mikz commented Jun 18, 2018

During master process boot all available policies are loaded
and get their init and init_worker phases executed.

Increases initial memory use by about 800 kB (which is later shared between workers).

@mikz mikz requested a review from a team as a code owner June 18, 2018 09:04
Loading policy multiple times returns the same instance.
@mikz mikz force-pushed the init-and-init-worker-policies branch from 5a528fd to 835ef42 Compare June 18, 2018 09:06
@mikz mikz force-pushed the init-and-init-worker-policies branch 2 times, most recently from 526ba5f to 6783a06 Compare June 18, 2018 15:07
Because it has to be executed for policies without configuration, it
needs to be executed on the module level, not instance.
@@ -99,7 +99,7 @@ init_worker_by_lua_block {
end)
end

require('apicast.executor'):init_worker()
require('apicast.executor').init_worker()
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be : right? because it applies to an instance of Executor.

@@ -79,7 +79,7 @@ init_by_lua_block {
package.loaded['apicast.executor'] = module
end

module:init()
module.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@mikz mikz force-pushed the init-and-init-worker-policies branch from 6783a06 to ebc62e5 Compare June 19, 2018 08:32
Now init() and init_worker() is run on service policies too.
@davidor davidor force-pushed the init-and-init-worker-policies branch from 7ed5ab8 to 096a9d4 Compare June 19, 2018 12:57
executed[policy.init_worker] = true
end

for _, policy in ipairs(policies or policy_loader:get_all()) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This caching complicates a lot testing this method. Depending on the execution the value will be cached or not and when it is, mocking policy_loader:get_all() will have no effect.
The tests I added in my latest commit are affected by this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you ok with not caching this @mikz ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. It would make more sense to me to provide reset for the cache than not having it at all.

I think it is important to be correct and that init and init_worker should be executed on exactly the same lists. Loading the list for the second time could not ensure that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@davidor davidor force-pushed the init-and-init-worker-policies branch from 096a9d4 to f8504bf Compare June 19, 2018 14:16
@mikz mikz changed the title [wip] init and init_worker phases called on all policies init and init_worker phases called on all policies Jun 19, 2018
@davidor davidor merged commit 46212ce into master Jun 19, 2018
@davidor davidor deleted the init-and-init-worker-policies branch June 19, 2018 14:39
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.

2 participants