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.

Sign up for GitHub

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

Jump to bottom

support gracefully shutdown #728

Merged
merged 1 commit into from
Dec 26, 2023
Merged

support gracefully shutdown #728

merged 1 commit into from
Dec 26, 2023

Conversation

jjz921024
Copy link
Contributor

for #727

Modifications:

  1. make a channel to receive quit signal
  2. add a atomic IsClosed indacate the reader should be close
  3. check IsClosed at the loop of SyncReader and ScanReader

Zheaoli
Zheaoli requested changes Dec 21, 2023
View reviewed changes
Copy link
Collaborator

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

以及建议格式化一下代码

internal/reader/parsing_aof.go Outdated
@@ -724,7 +725,7 @@ func (aofInfo *INFO) ParsingSingleAppendOnlyFile(FileName string, AOFTimeStamp i
log.Infof("Reading RDB Base File on AOF loading...")
rdbOpt := RdbReaderOptions{Filepath: AOFFilepath}
ldRDB := NewRDBReader(&rdbOpt)
ldRDB.StartRead()
ldRDB.StartRead(context.TODO())
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里尽量不要用 context.TODO

internal/reader/scan_standalone_reader.go Outdated
@@ -44,6 +46,8 @@ type scanStandaloneReader struct {
ScanPercentByDbId string `json:"scan_percent"`
NeedUpdateCount int64 `json:"need_update_count"`
}

isStop atomic.Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里为什么要用这个？

internal/reader/scan_standalone_reader.go Outdated
@@ -86,7 +94,7 @@ func (r *scanStandaloneReader) subscript() {
log.Panicf(err.Error())
}
regex := regexp.MustCompile(`\d+`)
for {
for !r.isStop.Load() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

尽量不要用这样的方式
可以在 for 循环里用 select 来解决

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是指这样吗？

for {
    select {
    case <-ch:
        goto Label
    default:
        // do something...
     }
 }
Label:

因为break不会跳出for循环，感觉golang这里设计的挺让人难受的，就没这么用...

Copy link
Collaborator

Choose a reason for hiding this comment

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

因为你这里没有剩下的逻辑，你 case <- ctx.Done 之后，直接进入 Close 逻辑，然后 Return 就行，不需要额外的处理

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的问题是如果你每次 for loop 都来 atomic Load 然后比较，会额外吃很多 CPU

Copy link
Contributor Author

Choose a reason for hiding this comment

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

明白了，我修改一下，谢谢~

internal/reader/sync_standalone_reader.go Outdated
@@ -223,7 +233,7 @@ func (r *syncStandaloneReader) receiveAOF(rd io.Reader) {
aofWriter := rotate.NewAOFWriter(r.stat.Name, r.stat.Dir, r.stat.AofReceivedOffset)
defer aofWriter.Close()
buf := make([]byte, 16*1024) // 16KB is enough for writing file
for {
for !r.isStop.Load() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上

@jjz921024
Copy link
Contributor Author

jjz921024 commented Dec 22, 2023 •
edited
Loading

@Zheaoli hi, I have fixed it, please check

Zheaoli
Zheaoli approved these changes Dec 22, 2023
View reviewed changes
Copy link
Collaborator

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM cc @suxb201

@Zheaoli
Copy link
Collaborator

Zheaoli commented Dec 22, 2023

测试挂了，得看下为什么

@jjz921024
Copy link
Contributor Author

测试挂了，得看下为什么

看了下日志，应该是CI的问题？

@suxb201
Copy link
Member

suxb201 commented Dec 25, 2023

我来修复下。

@suxb201
Copy link
Member

suxb201 commented Dec 25, 2023

@jjz921024 CI 修复了，需要你 rebase 代码再试一下。

@jjz921024
Copy link
Contributor Author

@jjz921024 CI 修复了，需要你 rebase 代码再试一下。

@suxb201 rebase了

@suxb201
Copy link
Member

suxb201 commented Dec 25, 2023

@jjz921024 还是有问题，我晚些看下。

suxb201
suxb201 requested changes Dec 25, 2023
View reviewed changes
internal/aof/aof.go Outdated Show resolved Hide resolved
@suxb201
Copy link
Member

suxb201 commented Dec 26, 2023

@jjz921024 辛苦

  1. 麻烦解决下冲突
  2. 麻烦合并下 9bb8985，尽量使用语意化的 commit msg：https://gist.github.com/joshbuchea/6f47e86d2510bce28f8e7f42ae84c716

@suxb201 suxb201 merged commit f7d72a2 into tair-opensource:v4 Dec 26, 2023
7 checks passed
@jjz921024 jjz921024 deleted the dev-close branch December 26, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suxb201 suxb201 suxb201 approved these changes

@Zheaoli Zheaoli Zheaoli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jjz921024 @Zheaoli @suxb201