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

add engine params to engine API #641

Merged
merged 4 commits into from
Jul 8, 2024
Merged

add engine params to engine API #641

merged 4 commits into from
Jul 8, 2024

Conversation

yuyang0
Copy link
Contributor

@yuyang0 yuyang0 commented Jul 6, 2024

enginefactory has two map, cache is cache key to engine API and keysToCheck is cache key to engineParams. this pr mainly does 2 things:

  1. add GetParams method to engine api, so we don't need to track engine params in engine factory
  2. remove keysToCheck

utils/cache.go Outdated
@@ -39,3 +39,12 @@ func (c *EngineCache) Get(endpoint string) engine.API {
func (c *EngineCache) Delete(host string, _ ...string) {
c.cache.Delete(host)
}

func (c *EngineCache) Items() map[string]engine.API {
Copy link
Contributor

Choose a reason for hiding this comment

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

唉为啥要强转回 Engine 实体

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个主要是为了保持一致,因为这个地方在go-cache包一层就是为了不用直接搞any,不过确实可以拿掉,少一次遍历,我其实想把这里的cache换成haxmap,cache主要的用途就是那个自动过期清理,但是按现在的实现代码已经明确清理了无效的engine,cache这个功能没啥必要了,而且ForEach遍历的时候就只遍历一次,go-cache用items来每一次都要遍历2次

Copy link
Contributor

Choose a reason for hiding this comment

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

我觉得你第二个思路可以……主要是吧,不想把 engine 内部细节暴露到外层来

@yuyang0
Copy link
Contributor Author

yuyang0 commented Jul 7, 2024

UT主要还是是那个并发的问题

}

// Get .
func (e *EngineCache) Get(key string) engine.API {
return e.cache.Get(key)
if api, found := e.cache.Get(key); found {
Copy link
Contributor

Choose a reason for hiding this comment

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

我记得默认行为没 key api 就为 nil 吧

@CMGS
Copy link
Contributor

CMGS commented Jul 7, 2024

LGTM

@CMGS CMGS merged commit 606f643 into projecteru2:master Jul 8, 2024
3 checks passed
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