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.

Sign up for GitHub

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

Jump to bottom

feature: Decoupled type #549

Merged
merged 75 commits into from
May 25, 2022
Merged

feature: Decoupled type #549

merged 75 commits into from
May 25, 2022

Conversation

akkw
Copy link
Contributor

@akkw akkw commented May 8, 2022 •
edited
Loading

What this PR does:

Which issue(s) this PR fixes:

Fixes #513

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

将COMPONENT NAME与具体的中间件类型解藕，在COMPONENT NAME块中KEY:VAKLUE对新增type:name (name为中间件类型)

image

@seeflood seeflood requested a review from zhenjunMa May 10, 2022 03:07
@seeflood
Copy link
Member

@zhenjunMa Please help review this PR

akkw added 2 commits May 11, 2022 19:33
@akkw
Merge branch 'main' of github.com:mosn/layotto into decoupled_type
1480708 
� Conflicts:
�	demo/configuration/common/client.go
�	demo/configuration/etcd/etcd.go
�	demo/state/redis/client.go
@akkw
decouple Name and Type
aefe748
wenxuwan
wenxuwan requested changes May 11, 2022
View reviewed changes
Copy link
Member

@wenxuwan wenxuwan left a comment

Choose a reason for hiding this comment

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

这个改动要考虑兼容问题？如果用户升级了Layotto，但配置文件没变，那是不是加载就出问题，可以做成如果没配置type字段就用原先的作为type名字

@akkw
Copy link
Contributor Author

akkw commented May 11, 2022

这个改动要考虑兼容问题？如果用户升级了Layotto，但配置文件没变，那是不是加载就出问题，可以做成如果没配置type字段就用原先的作为type名字

这样子我感觉对于存量用户是不是配置文件会比较混乱啊，统一性没有标准。
其次，这个改动其实对于配置文件来说改动不是很大甚至我们可以实现一个小工具去帮助用户升级配置文件，这些我觉得难度都不是很大

@wenxuwan
Copy link
Member

这个改动要考虑兼容问题？如果用户升级了Layotto，但配置文件没变，那是不是加载就出问题，可以做成如果没配置type字段就用原先的作为type名字

这样子我感觉对于存量用户是不是配置文件会比较混乱啊，统一性没有标准。 其次，这个改动其实对于配置文件来说改动不是很大甚至我们可以实现一个小工具去帮助用户升级配置文件，这些我觉得难度都不是很大

这个问题不能用升级难不难来说，而是我们作为开源项目，要做到向前兼容性。比如我现在引用了layotto,但我升级layotto最新版本，我配置文件并不知道需要更改。然后发上线发现挂掉了。

@zhenjunMa
Copy link
Contributor

这个改动要考虑兼容问题？如果用户升级了Layotto，但配置文件没变，那是不是加载就出问题，可以做成如果没配置type字段就用原先的作为type名字

这样子我感觉对于存量用户是不是配置文件会比较混乱啊，统一性没有标准。 其次，这个改动其实对于配置文件来说改动不是很大甚至我们可以实现一个小工具去帮助用户升级配置文件，这些我觉得难度都不是很大

这个问题不能用升级难不难来说，而是我们作为开源项目，要做到向前兼容性。比如我现在引用了layotto,但我升级layotto最新版本，我配置文件并不知道需要更改。然后发上线发现挂掉了。

这里其实之前讨论过，主要是考虑到现在 Layotto 还属于相对早一点的阶段，影响范围可控，不想搞那么多兼容代码😜

zhenjunMa
zhenjunMa previously approved these changes May 12, 2022
View reviewed changes
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.

LGTM, thanks!

@seeflood seeflood added this to the v0.5 milestone May 12, 2022
@seeflood
Copy link
Member

We can merge this PR after v0.4 released.

seeflood
seeflood requested changes May 12, 2022
View reviewed changes
Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

Since this PR introduce an incompatible change, please add more description in the release note.
Github will show your description when releasing new version
image

seeflood
seeflood requested changes May 12, 2022
View reviewed changes
Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

现在 quickstart hang住了，因为demo 里传的 storename 是"etcd"，但是 json 里的 name被改掉了~需要修复下哈
https://github.com/mosn/layotto/runs/6401619750?check_suite_focus=true
image

seeflood
seeflood requested changes May 12, 2022
View reviewed changes
configs/config_lock_redis.json Outdated Show resolved Hide resolved
seeflood
seeflood requested changes May 12, 2022
View reviewed changes
configs/config_apollo_health.json Outdated Show resolved Hide resolved
@akkw
Copy link
Contributor Author

akkw commented May 13, 2022

现在 quickstart hang住了，因为demo 里传的 storename 是"etcd"，但是 json 里的 name被改掉了~需要修复下哈 https://github.com/mosn/layotto/runs/6401619750?check_suite_focus=true image

好的，我去看一下 之前好像没有发现这个问题

@seeflood
fix ut
8a0eda9 
Signed-off-by: seeflood <zhou.qunli@foxmail.com>
seeflood
seeflood requested changes May 23, 2022
View reviewed changes
sdk/go-sdk/client/client_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/client_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/client_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/go.mod Outdated Show resolved Hide resolved
@seeflood
Copy link
Member

seeflood commented May 24, 2022 •
edited
Loading

@akkw 要不我直接在你的分支修改、尽快合并吧，我担心再不合并会有各种冲突，要花很多时间修冲突~
方便记个 “补充文档” 的 issue么，文档可以后面再提 pr 修

akkw added 9 commits May 24, 2022 19:29
seeflood
seeflood requested changes May 24, 2022
View reviewed changes
components/custom/registry.go Outdated Show resolved Hide resolved
components/custom/registry.go Outdated Show resolved Hide resolved
components/custom/registry.go Show resolved Hide resolved
seeflood
seeflood requested changes May 24, 2022
View reviewed changes
demo/sequencer/common/client.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
seeflood
seeflood requested changes May 25, 2022
View reviewed changes
pkg/runtime/runtime.go Show resolved Hide resolved
sdk/go-sdk/client/client_test.go Show resolved Hide resolved
sdk/go-sdk/client/client_test.go Show resolved Hide resolved
sdk/go-sdk/client/client_test.go Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
sdk/go-sdk/client/lock_test.go Outdated Show resolved Hide resolved
@seeflood
fix ut
405a271 
Signed-off-by: seeflood <zhou.qunli@foxmail.com>
@seeflood
Copy link
Member

@mosn/layotto-commiter @mosn/layotto-pmc 这个 PR 先帮忙合并吧，一些遗留小问题不影响功能，我提新的PR修复。正好我要重新跑一遍 linter 检查

seeflood
seeflood approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

lgtm

@seeflood seeflood merged commit 37312d8 into mosn:main May 25, 2022
@seeflood
Copy link
Member

感谢贡献！！！辛苦了！

@seeflood seeflood mentioned this pull request May 25, 2022
Merged
seeflood added a commit that referenced this pull request May 25, 2022
@seeflood
fix: fix legacy issues of #549 (#604)
7a2eefd 
Signed-off-by: seeflood <zhou.qunli@foxmail.com>
@seeflood seeflood mentioned this pull request Jul 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seeflood seeflood seeflood approved these changes

@wenxuwan wenxuwan wenxuwan approved these changes

@zhenjunMa zhenjunMa zhenjunMa left review comments

Assignees

@seeflood seeflood

Labels
area/community cla:yes kind/incompatible size/XXL
Projects
None yet
Milestone
v0.5
Development

Successfully merging this pull request may close these issues.

解耦API跟具体实现
6 participants
@akkw @seeflood @wenxuwan @zhenjunMa @azhsmesos @Xunzhuo