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

optimize: latencyString shows realLatency(+offset) #307

Merged
merged 5 commits into from
Sep 2, 2023

Conversation

luochen1990
Copy link
Contributor

Background

this PR make latencyString() shows latency like realLatency(+offset) instead of latencyAfterOffset(latencyBeforeOffset)

Checklist

Full Changelogs

  • [Implement ...]

Issue Reference

Closes #[issue number]

Test Result

@mzz2017 mzz2017 changed the title feat: latencyString shows realLatency(+offset) optimize: latencyString shows realLatency(+offset) Sep 2, 2023
Copy link
Contributor

@mzz2017 mzz2017 left a comment

Choose a reason for hiding this comment

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

虽然可以……但为什么每次都要把 map 里的 offset 取出来存进 o 成员里?

@luochen1990 luochen1990 force-pushed the feature-reimpl-latency-offset branch from c94f568 to 268f3e7 Compare September 2, 2023 09:16
@luochen1990
Copy link
Contributor Author

虽然可以……但为什么每次都要把 map 里的 offset 取出来存进 o 成员里?

umm, 不确定你指的具体是哪一行。

如果是指 127~128 行的话, 那是因为 l 和 o 在排序和显示的时候都会用到,所以就直接放进 alive 里?

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

直接用 offset 变量也可以?

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

如果是显示的话,可以调 map 再拿一遍

@luochen1990
Copy link
Contributor Author

直接用 offset 变量也可以?

具体是哪一段,可以指定行评论咩?

@luochen1990 luochen1990 marked this pull request as ready for review September 2, 2023 09:51
@luochen1990 luochen1990 requested a review from a team as a code owner September 2, 2023 09:51
@luochen1990 luochen1990 force-pushed the feature-reimpl-latency-offset branch from f290690 to e2df89e Compare September 2, 2023 09:52
mzz2017
mzz2017 previously approved these changes Sep 2, 2023
Copy link
Contributor

@mzz2017 mzz2017 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

image 经过测试,排名第三的节点有着更低的时延。但它似乎并不能很直观地显示出来最终的时延和排名是什么样的。如果能够在排序的时候有更直观的方式,例如: 1. Node 1 (727ms=227ms+500ms) 我认为是更直观的。

另外,在我勾选的结果来看,它选择了第三个节点,这似乎是非预期行为?

@luochen1990
Copy link
Contributor Author

Node 1 (727ms=227ms+500ms)

Node 1 227ms(+500ms=727ms) 这样展示你觉得如何?

@luochen1990
Copy link
Contributor Author

它选择了第三个节点,这似乎是非预期行为?

看起来是的, 我不确定是否是我这个变更引入的问题,请问我应该怎么本地测试比较推荐?

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

Node 1 (727ms=227ms+500ms)

Node 1 227ms(+500ms=727ms) 这样展示你觉得如何?

可以。

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

它选择了第三个节点,这似乎是非预期行为?

看起来是的, 我不确定是否是我这个变更引入的问题,请问我应该怎么本地测试比较推荐?

这是因为 re-selects 的时候是使用的 .l 而不是 .l+.o,请检查一下所有的引用。

实际上,我们没有必要改变之前的行为,没有必要引入 .o,只需要去掉 epsilon 那部分的 >0 判断即可。

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

本地测试:

make ebpf
go run -exec sudo . run -c /etc/dae/config.dae

@luochen1990
Copy link
Contributor Author

这是因为 re-selects 的时候是使用的 .l 而不是 .l+.o,请检查一下所有的引用。

实际上,我们没有必要改变之前的行为,没有必要引入 .o,只需要去掉 epsilon 那部分的 >0 判断即可。

OK, 我尝试修复一下

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

@luochen1990 我看了一下代码,因为有 clamp(钳制到 Timeout 以内),所以想要 reverse 回去按照原来的思路似乎是不可行的。需要保存之前的 latency,使用你的这套代码是正确的。

@luochen1990
Copy link
Contributor Author

因为 re-selects 的时候是使用的 .l 而不是 .l+.o,请检查一下所有的引用

确认一下, re-selects 是在 calcMinLatency 这里么?

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

因为 re-selects 的时候是使用的 .l 而不是 .l+.o,请检查一下所有的引用

确认一下, re-selects 是在 calcMinLatency 这里么?

是的。

@luochen1990
Copy link
Contributor Author

我感觉可能需要重构一下, 因为涉及 latency 的地方有点多,我打算引入一个 SortingLatency 的计算属性,来替代之前引用 latency 的地方,可能是像这样:

func (a *AliveDialerSet) SortingLatency(d *Dialer) time.Duration {
	return a.dialerToLatency[d] + a.dialerToLatencyOffset[d]
}

你觉得 OK 么? 因为这个动作可能有点大, 我不确定你觉得是否OK

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

@luochen1990 没问题

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

image

LGTM. 待 PR Build #307 运行完毕后,你也可以测试一下。

@mzz2017 mzz2017 merged commit eaf9684 into daeuniverse:main Sep 2, 2023
@mzz2017
Copy link
Contributor

mzz2017 commented Sep 2, 2023

LGTM. 非常感谢!

@luochen1990
Copy link
Contributor Author

感谢你分享这么好用的软件, 非常荣幸能贡献代码 :)

Copy link
Contributor

@dae-prow dae-prow bot left a comment

Choose a reason for hiding this comment

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

🧪 Since the PR has been fully tested, please consider merging it.

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.

3 participants