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 config command #181

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

add config command #181

wants to merge 21 commits into from

Conversation

cheny-alf
Copy link

No description provided.

config/config.go Outdated Show resolved Hide resolved
if len(args)%2 != 0 {
return protocol.MakeErrReply("ERR wrong number of arguments for 'config|set' command")
}
properties := config.CopyProperties()
Copy link
Owner

Choose a reason for hiding this comment

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

不懂为什么需要 copy

Copy link
Author

Choose a reason for hiding this comment

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

因为config set是原子性的,所以我希望拷贝一份配置对象,然后先在这个对象上进行修改,最后的时候再将原本的配置文件对象指向这个修改后的对象

Copy link
Owner

Choose a reason for hiding this comment

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

因为线程可见性的原因,在没有使用 mutex 或 atomic 等并发原语的情况下是无法保证原子性的。建议了解一下 happens-before, 线程可见性和指令重拍等内容

Copy link
Author

Choose a reason for hiding this comment

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

这里我加了个mutex,或者可以给个别的思路嘛,如何保证同时多个配置时的原子性

database/config.go Outdated Show resolved Hide resolved
database/config.go Show resolved Hide resolved
if len(args)%2 != 0 {
return protocol.MakeErrReply("ERR wrong number of arguments for 'config|set' command")
}
properties := config.CopyProperties()
Copy link
Owner

Choose a reason for hiding this comment

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

因为线程可见性的原因,在没有使用 mutex 或 atomic 等并发原语的情况下是无法保证原子性的。建议了解一下 happens-before, 线程可见性和指令重拍等内容

database/config.go Outdated Show resolved Hide resolved
database/config.go Outdated Show resolved Hide resolved
database/config.go Show resolved Hide resolved
@cheny-alf cheny-alf requested a review from HDT3213 June 6, 2023 05:16
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