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: load multi wasm plugins #182

Merged
merged 28 commits into from
Aug 20, 2021
Merged

feat: load multi wasm plugins #182

merged 28 commits into from
Aug 20, 2021

Conversation

zu1k
Copy link
Member

@zu1k zu1k commented Aug 13, 2021

What this PR does:

Let layotto support loading multi wasm plugins

Which issue(s) this PR fixes:

Fixes #176

Special notes for your reviewer:

Idea validation, not completed, encountered major obstacles

Does this PR introduce a user-facing change?:

config stream_filters changed

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2021

Codecov Report

Merging #182 (b9dc565) into main (17552a5) will decrease coverage by 1.67%.
The diff coverage is 2.10%.

❗ Current head b9dc565 differs from pull request most recent head a18e4e1. Consider uploading reports for the commit a18e4e1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
- Coverage   58.24%   56.56%   -1.68%     
==========================================
  Files          50       51       +1     
  Lines        2148     2215      +67     
==========================================
+ Hits         1251     1253       +2     
- Misses        775      841      +66     
+ Partials      122      121       -1     
Impacted Files Coverage Δ
pkg/wasm/abi.go 8.33% <0.00%> (-16.67%) ⬇️
pkg/wasm/dispatch.go 0.00% <0.00%> (ø)
pkg/wasm/factory.go 2.06% <0.00%> (-0.12%) ⬇️
pkg/wasm/filter.go 0.00% <0.00%> (ø)
pkg/wasm/imports.go 0.00% <0.00%> (ø)
pkg/wasm/watcher.go 8.64% <0.00%> (+0.10%) ⬆️
pkg/wasm/config.go 48.93% <100.00%> (ø)
...kg/filter/network/tcpcopy/persistence/work_pool.go 90.19% <0.00%> (+3.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17552a5...a18e4e1. Read the comment docs.

@zhenjunMa zhenjunMa self-requested a review August 17, 2021 13:18
demo/wasm/code/golang/wasm.go Show resolved Hide resolved
demo/wasm/config.json Show resolved Hide resolved
pkg/wasm/abi.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Outdated Show resolved Hide resolved
@zu1k zu1k requested a review from zhenjunMa August 19, 2021 09:22
@zu1k zu1k marked this pull request as ready for review August 19, 2021 11:24
pkg/wasm/dispatch.go Outdated Show resolved Hide resolved
pkg/wasm/factory.go Outdated Show resolved Hide resolved
pkg/wasm/factory.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Outdated Show resolved Hide resolved
pkg/wasm/factory.go Show resolved Hide resolved
pkg/wasm/factory.go Outdated Show resolved Hide resolved
pkg/wasm/factory.go Outdated Show resolved Hide resolved
pkg/wasm/filter.go Show resolved Hide resolved
@zhenjunMa
Copy link
Contributor

整体思路完全正确👍,部分细节可以再讨论下

@zu1k zu1k requested a review from zhenjunMa August 20, 2021 04:29
@zu1k
Copy link
Member Author

zu1k commented Aug 20, 2021

一开始想改成加权轮训,不过发现把注册路由放到OnPluginStart里其实会每一个instance都注册一次,实际上已经等于是加权了,所以又改回随机了

也就是说getID这个行为,每一个wasm都会运行instanceNum次,我觉得还是放在原来的位置好

@zhenjunMa
Copy link
Contributor

是的,放在createProxyWasmFilterFactory会感觉代码有冗余,放在OnPluginStart又会按照instance维度反复获取ID,好在这种方式会把请求自动打散在Instance粒度, 两种写法都有瑕疵,感觉可能我们在wasm生命周期管理这块还有优化的空间。

Copy link
Contributor

@zhenjunMa zhenjunMa left a comment

Choose a reason for hiding this comment

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

非常感谢贡献如此重要的feature🍺

@zhenjunMa zhenjunMa closed this Aug 20, 2021
@zhenjunMa zhenjunMa reopened this Aug 20, 2021
@zhenjunMa zhenjunMa closed this Aug 20, 2021
@zhenjunMa zhenjunMa reopened this Aug 20, 2021
@zhenjunMa zhenjunMa closed this Aug 20, 2021
@zhenjunMa zhenjunMa reopened this Aug 20, 2021
@zhenjunMa zhenjunMa merged commit 742c3e9 into mosn:main Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New feature: load multi wasm instance
3 participants