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

[dbnode] DB Close Wait for Fs Processes #2229

Merged
merged 4 commits into from
Mar 28, 2020

Conversation

notbdu
Copy link
Contributor

@notbdu notbdu commented Mar 26, 2020

What this PR does / why we need it:

Waits for fs processes to finish before closing db. Also checks db state before running a tick of fs processes.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@notbdu notbdu requested a review from robskillington March 26, 2020 03:25
tickCheckInterval = 5 * time.Second
fileOpCheckInterval = time.Second
tickCheckInterval = 5 * time.Second
completionCheckInterval = 1 * time.Second
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could even make this every 100ms since it only happens on close. I remember the kill signal only gives us 10s to exit so the faster the better I suppose.

"ongoing tick was unable to release time barrier", zap.Error(err))
continue
log.Error("ongoing tick was unable to release time barrier", zap.Error(err))
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, think this should still be a continue yeah?

}

func (m *mediator) databaseState() databaseState {
db := m.database.(*db)
Copy link
Collaborator

@robskillington robskillington Mar 26, 2020

Choose a reason for hiding this comment

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

Hm this seems racy without acquiring a lock, also better to add to interface rather than casting to *db.

Could you add a public method to type Database interface just called IsOpen() bool?

That way we can do the read lock from there too.

@robskillington robskillington changed the title DB Close Wait for Fs Processes [dbnode] DB Close Wait for Fs Processes Mar 26, 2020
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #2229 into r/fail-build-with-flush-erros will increase coverage by 1.2%.
The diff coverage is 85.9%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           r/fail-build-with-flush-erros   #2229     +/-   ##
===============================================================
+ Coverage                           71.1%   72.4%   +1.2%     
===============================================================
  Files                               1021    1022      +1     
  Lines                              88855   88892     +37     
===============================================================
+ Hits                               63254   64366   +1112     
+ Misses                             21258   20190   -1068     
+ Partials                            4343    4336      -7     
Flag Coverage Δ
#aggregator 82.1% <ø> (+<0.1%) ⬆️
#cluster 85.4% <ø> (+0.1%) ⬆️
#collector 82.8% <ø> (ø)
#dbnode 78.9% <85.9%> (+2.9%) ⬆️
#m3em 74.4% <ø> (ø)
#m3ninx 74.4% <ø> (+<0.1%) ⬆️
#m3nsch 51.1% <ø> (ø)
#metrics 17.5% <ø> (ø)
#msg 74.9% <ø> (ø)
#query 68.9% <ø> (ø)
#x 83.3% <ø> (+<0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd55a30...7dbc6ee. Read the comment docs.

@notbdu notbdu force-pushed the bdu/wait-fs-processes branch 2 times, most recently from a7a157a to 3988202 Compare March 27, 2020 01:11
@notbdu notbdu force-pushed the bdu/wait-fs-processes branch from 3988202 to c5cdfc5 Compare March 27, 2020 01:12
Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@notbdu notbdu merged commit b48997f into r/fail-build-with-flush-erros Mar 28, 2020
notbdu pushed a commit that referenced this pull request Mar 28, 2020
…rrors in integration tests (#2217)

* [dbnode] Fail builds with flush errors in integration tests

* [dbnode] DB Close Wait for Fs Processes (#2229)

* [dbnode] Only read data from disk when flushing index segments.
@justinjc justinjc deleted the bdu/wait-fs-processes branch May 14, 2020 16:14
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