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

feat: provides logging and hook support for redis #74

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

mutezebra
Copy link
Member

No description provided.

@mutezebra mutezebra requested review from SchwarzSail and removed request for ozline November 1, 2024 03:37
pkg/logger/redis.go Outdated Show resolved Hide resolved
pkg/constants/constants.go Outdated Show resolved Hide resolved
@@ -31,11 +33,17 @@ func NewRedisClient(db int) (*redis.Client, error) {
if config.Redis == nil {
return nil, errors.New("redis config is nil")
}

l := logger.GetRedisLogger()
redis.SetLogger(l)
Copy link
Member

Choose a reason for hiding this comment

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

这个setlogger在new之前不会有问题么?

我没有注意到还有这个logger,想问一下这个和我们hook有什么区别?

Copy link
Member Author

Choose a reason for hiding this comment

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

一个进程只会有一个loggerObj,其他的任何logger都是对他的封装。RedisLogger也一样,只是加了四个方法来满足redis定义的接口

Copy link
Member Author

Choose a reason for hiding this comment

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

client中没有logger对象,client的日志输出就是调用的全局logger,所以不会有问题

Copy link
Member

Choose a reason for hiding this comment

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

不是啊,我是想问你 new 这个对象之前就 setlogger 了,会不会有点问题?需要放到后面吗

Copy link
Member Author

Choose a reason for hiding this comment

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

不会有问题。但是在逻辑上确实应该放在初始化之后,改了

jiuxia211
jiuxia211 previously approved these changes Nov 4, 2024
@SchwarzSail
Copy link
Member

LGTM

SchwarzSail
SchwarzSail previously approved these changes Nov 4, 2024
@ozline
Copy link
Member

ozline commented Nov 4, 2024

你新的 commit 还是没有签名

@mutezebra
Copy link
Member Author

你新的 commit 还是没有签名

有了有了

Copy link
Member

@SchwarzSail SchwarzSail left a comment

Choose a reason for hiding this comment

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

LGTM

@SchwarzSail SchwarzSail merged commit 57d94f3 into west2-online:main Nov 4, 2024
6 checks passed
@mutezebra mutezebra deleted the redis-log branch November 4, 2024 13:22
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.

4 participants