-
Notifications
You must be signed in to change notification settings - Fork 701
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: use freecache #416
feat: use freecache #416
Conversation
可以补下单测 |
可以放在pkg/cache/freecache,方便后面增加其他cache实现 |
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
==========================================
- Coverage 40.29% 39.97% -0.33%
==========================================
Files 84 84
Lines 5611 5611
==========================================
- Hits 2261 2243 -18
- Misses 3192 3211 +19
+ Partials 158 157 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pkg/cache/localcache.go
Outdated
// metric上报 | ||
if !l.req.DisableMetric { | ||
defer func() { | ||
prome.LocalCacheHandleHistogram.WithLabelValues(prome.TypeLocalCache, l.req.Name, "EntryCount").Observe(float64(l.cache.EntryCount())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里不用defer,可以放在Set之后
pkg/cache/localcache.go
Outdated
// metric上报 | ||
if !l.req.DisableMetric { | ||
defer func() { | ||
prome.LocalCacheHandleHistogram.WithLabelValues(prome.TypeLocalCache, l.req.Name, "MissCount").Observe(float64(l.cache.MissCount())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
类似的
pkg/cache/cache.go
Outdated
}() | ||
|
||
func() { | ||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recover可以移除,应该在外层捕获异常
pkg/cache/localcache.go
Outdated
Cache | ||
} | ||
|
||
type LocalCacheReq struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可以参考下其他组件的模式,通过Config实现Build能力
pkg/cache/localcache.go
Outdated
xlog.Jupiter().Panic("cache NewLocalCache expire err", zap.Any("req", req)) | ||
} | ||
if len(req.Name) == 0 { | ||
req.Name = fmt.Sprintf("cache-%d", time.Now().UnixNano()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里可以考虑基于uuid生成一个唯一key
pkg/cache/xerr/err.go
Outdated
@@ -0,0 +1,36 @@ | |||
package xerr | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
去掉recover逻辑后,这个文件应该可以删掉
pkg/cache/xfreecache/localcache.go
Outdated
data, err = l.cache.Get([]byte(key)) | ||
// metric report | ||
if !l.req.DisableMetric { | ||
prome.CacheHandleGauge.WithLabelValues(prome.TypeLocalCache, l.req.Name, "HitRate").Set(l.cache.HitRate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
看上去这个每次都会遍历几百个segments,感觉可能会对效率有影响
如果自己来统计这个是不是会更高效呢?可以考虑写个benchmark看看
Describe what this PR does / why we need it
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews