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

Config: Remove global transport #3751

Merged
merged 10 commits into from
Sep 5, 2024
Merged

Config: Remove global transport #3751

merged 10 commits into from
Sep 5, 2024

Conversation

Fangliding
Copy link
Member

尝试移除全局TransportConfig 本来以为会有点史不过还是挺干净的 代码里分的很明显叫TransportConfig和StreamConfig 也只在处理config时候作用没影响别的part

@RPRX
Copy link
Member

RPRX commented Sep 2, 2024

这种移除后要加个报错

@Fangliding
Copy link
Member Author

Fangliding commented Sep 2, 2024

嗯 好像言出法随不小心改炸了 再看看

@Fangliding
Copy link
Member Author

Fangliding commented Sep 2, 2024

这种移除后要加个报错

ray本身的机制一直是无视多余的选项 当然额外加一个检测到某个字段就panic也行 个人觉得不是很有必要 本来这就是个没人用的功能 可能有的老旧工具或者config会有这个字段但是不会加入有意义内容 那样就误杀了

@RPRX
Copy link
Member

RPRX commented Sep 2, 2024

有人用了曾经有过的功能必须要报错退出,或者你可以写成这个字段里有实际内容才报错

@Fangliding
Copy link
Member Author

有人用了曾经有过的功能必须要报错退出,或者你可以写成这个字段里有实际内容才报错

那还得保留这个build(((

@RPRX
Copy link
Member

RPRX commented Sep 2, 2024

我记得可以用个 json raw 啥的结构接收,然后再作分析,判断有没有任何内容

@RPRX
Copy link
Member

RPRX commented Sep 2, 2024

json.RawMessage

@Fangliding
Copy link
Member Author

json.RawMessage

没耍过 晚上有时间看下吧

@mmmray mmmray mentioned this pull request Sep 2, 2024
@RPRX
Copy link
Member

RPRX commented Sep 3, 2024

json.RawMessage

@mmmray 你有空的话顺便改一下吧

@RPRX
Copy link
Member

RPRX commented Sep 3, 2024

Conflict

@@ -22,84 +20,9 @@ type TransportConfig struct {
func (c *TransportConfig) Build() (*global.Config, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like this function is never called. I deleted this entire file and replaced TransportConfig with json.RawValue, and it compiled fine

Copy link
Member

Choose a reason for hiding this comment

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

it seems like this function is never called. I deleted this entire file and replaced TransportConfig with json.RawValue, and it compiled fine

可以

@RPRX
Copy link
Member

RPRX commented Sep 4, 2024

@mmmray 要不使用非指针?现在这种写法,如果仅有这个字段但里面没任何内容也会报错吧?我觉得要防止

可能有的老旧工具或者config会有这个字段但是不会加入有意义内容 那样就误杀了

@mmmray
Copy link
Collaborator

mmmray commented Sep 4, 2024

How about this then? I don't really know what the old tools do, but if it's "transport": {}, then this should be sufficient. If not then fangliding is right and the struct needs to be restored.

@RPRX
Copy link
Member

RPRX commented Sep 4, 2024

but if it's "transport": {}, then this should be sufficient.

对,测试了吗

@Fangliding
Copy link
Member Author

我较劲了半天的test直接被删掉了 虽然那东西个人感觉确实没用

@Fangliding
Copy link
Member Author

@mmmray 要不使用非指针?现在这种写法,如果仅有这个字段但里面没任何内容也会报错吧?我觉得要防止

可能有的老旧工具或者config会有这个字段但是不会加入有意义内容 那样就误杀了

我觉得可以release之后看反馈多不多再说

@RPRX
Copy link
Member

RPRX commented Sep 4, 2024

直接 json.RawMessage 的话,内容包不包括外面的 "transport": {}

@mmmray
Copy link
Collaborator

mmmray commented Sep 4, 2024

This is tested:

The configs {"transport": {}} and {} don't raise errors

Things like {"transport": {"tcpSettings": {}}} or {"transport": {"randomkey": true}} still raise deprecation errors

All of them are "not meaningful", I don't know which one exists in practice

@RPRX RPRX changed the title Config: Remove global transport config Config: Remove global transport Sep 5, 2024
@RPRX RPRX merged commit 84d3eb0 into main Sep 5, 2024
36 checks passed
@RPRX
Copy link
Member

RPRX commented Sep 5, 2024

@mmmray 你把这个 commit 改成作者为你,Co-authored-by @Fangliding

yuhan6665 pushed a commit that referenced this pull request Sep 6, 2024
yuhan6665 pushed a commit that referenced this pull request Sep 6, 2024
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.

3 participants