TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 108 Pull requests 3 Projects Releases 1 Activity Actions

Init write-cache asynchronously #35

Merged
carpawell merged 2 commits from carpawell/faster-wc-init into master 2023-03-09 11:07:34 +00:00
Conversation 1 Commits 2 Files changed 9 +133 -15
carpawell commented 2023-01-27 15:38:11 +00:00 (Migrated from github.com)
Copy link
No description provided.
fyrchik (Migrated from github.com) reviewed 2023-01-27 15:38:11 +00:00
ale64bit (Migrated from github.com) reviewed 2023-01-27 15:38:11 +00:00
fyrchik (Migrated from github.com) reviewed 2023-01-31 08:45:23 +00:00
CHANGELOG.md
fyrchik (Migrated from github.com) commented 2023-01-31 08:30:12 +00:00
Copy link

Initialize write-cache asynchronously?

`Initialize write-cache asynchronously`?
pkg/local_object_storage/writecache/init.go
fyrchik (Migrated from github.com) commented 2023-01-31 08:43:54 +00:00
Copy link

Can we do it in defer, just in case?

Can we do it in `defer`, just in case?
fyrchik (Migrated from github.com) commented 2023-01-31 08:44:33 +00:00
Copy link

Can you add a comment about why we don't need a mutex here?

Can you add a comment about why we don't need a mutex here?
pkg/local_object_storage/writecache/init.go
@ -16,0 +39,4 @@
localWG.Wait()
select {
case <-c.stopInitCh:
fyrchik (Migrated from github.com) commented 2023-01-31 08:45:12 +00:00
Copy link

I think this should be added in c.wg too, no?

I think this should be added in `c.wg` too, no?
pkg/local_object_storage/writecache/writecache.go
fyrchik (Migrated from github.com) commented 2023-01-31 08:43:02 +00:00
Copy link

initialized?

`initialized`?
pkg/local_object_storage/writecache/writecache.go
fyrchik (Migrated from github.com) commented 2023-01-31 08:42:19 +00:00
Copy link

Please, let's just make it "always async". If we ever need to configure, we can simply add WithAsyncInitialization option.

Please, let's just make it "always async". _If we ever need to configure_, we can simply add `WithAsyncInitialization` option.
acid-ant (Migrated from github.com) reviewed 2023-01-31 09:06:53 +00:00
carpawell (Migrated from github.com) reviewed 2023-01-31 12:20:03 +00:00
pkg/local_object_storage/writecache/init.go
@ -16,0 +39,4 @@
localWG.Wait()
select {
case <-c.stopInitCh:
carpawell (Migrated from github.com) commented 2023-01-31 12:20:03 +00:00
Copy link

hw, yeah, would be more reliable, added

hw, yeah, would be more reliable, added
carpawell (Migrated from github.com) reviewed 2023-01-31 12:20:05 +00:00
pkg/local_object_storage/writecache/init.go
carpawell (Migrated from github.com) commented 2023-01-31 12:20:04 +00:00
Copy link

added but depends on async flag discussion (seems like it could be even removed)

added but depends on async flag discussion (seems like it could be even removed)
carpawell (Migrated from github.com) reviewed 2023-01-31 12:20:09 +00:00
pkg/local_object_storage/writecache/writecache.go
carpawell (Migrated from github.com) commented 2023-01-31 12:20:08 +00:00
Copy link

longer but dont mind

longer but dont mind
carpawell (Migrated from github.com) reviewed 2023-01-31 12:20:09 +00:00
pkg/local_object_storage/writecache/writecache.go
carpawell (Migrated from github.com) commented 2023-01-31 12:20:09 +00:00
Copy link

yes, that's an ugly change. problems appear when you try to (unit)test it. WriteCache should either be able to init in sync mode or have a feedback channel to signal about its ready state then. i decided to open PR anyway and discuss that. your thoughts?
/cc @acid-ant

yes, that's an ugly change. problems appear when you try to (unit)test it. `WriteCache` should either be able to init in sync mode or have a feedback channel to signal about its ready state then. i decided to open PR anyway and discuss that. your thoughts? /cc @acid-ant
acid-ant (Migrated from github.com) reviewed 2023-01-31 12:41:48 +00:00
pkg/local_object_storage/writecache/writecache.go
acid-ant (Migrated from github.com) commented 2023-01-31 12:41:48 +00:00
Copy link

vote for new option for writecache WithAsyncInitialization and init it in background by default.

vote for new option for writecache `WithAsyncInitialization` and init it in background by default.
fyrchik (Migrated from github.com) reviewed 2023-01-31 13:37:43 +00:00
pkg/local_object_storage/writecache/writecache.go
fyrchik (Migrated from github.com) commented 2023-01-31 13:37:42 +00:00
Copy link

Can we just use require.Eventually in tests?

Can we just use `require.Eventually` in tests?
carpawell (Migrated from github.com) reviewed 2023-02-01 20:59:37 +00:00
pkg/local_object_storage/writecache/writecache.go
carpawell (Migrated from github.com) commented 2023-02-01 20:59:36 +00:00
Copy link

Can we just use require.Eventually in tests?

yes, but with magic numbers (how often and how long wait for that status (and how? try to Put object? export Status func?)). do we want it?

vote for new option for writecache WithAsyncInitialization and init it in background by default.

we have Init -> Close -> Init scenario. i could imagine that sync/async initialization could be a config value in future. also, IMO creation option should not change Init behavior

> Can we just use require.Eventually in tests? yes, but with magic numbers (how often and how long wait for that status (and how? try to `Put` object? export `Status` func?)). do we want it? > vote for new option for writecache WithAsyncInitialization and init it in background by default. we have `Init` -> `Close` -> `Init` scenario. i could imagine that sync/async initialization could be a config value in future. also, IMO creation option should not change `Init` behavior
fyrchik (Migrated from github.com) reviewed 2023-02-02 16:32:15 +00:00
pkg/local_object_storage/writecache/writecache.go
fyrchik (Migrated from github.com) commented 2023-02-02 16:32:15 +00:00
Copy link

do we want it?

We already have it in some places, I don't mind having it here.

i could imagine that sync/async initialization could be a config value in future

Well, it could, but we don't need it now.

creation option should not change Init behavior

Why so?

> do we want it? We already have it in some places, I don't mind having it here. > i could imagine that sync/async initialization could be a config value in future Well, it could, but we don't _need_ it now. > creation option should not change Init behavior Why so?
carpawell (Migrated from github.com) reviewed 2023-02-15 14:51:29 +00:00
pkg/local_object_storage/writecache/writecache.go
carpawell (Migrated from github.com) commented 2023-02-15 14:51:29 +00:00
Copy link

I don't mind having it here

ok, dropped async flag

> I don't mind having it here ok, dropped `async` flag
acid-ant (Migrated from github.com) reviewed 2023-02-16 05:45:47 +00:00
fyrchik (Migrated from github.com) reviewed 2023-02-16 06:14:58 +00:00
CHANGELOG.md
fyrchik (Migrated from github.com) commented 2023-02-16 06:08:06 +00:00
Copy link

2 spaces here

2 spaces here
pkg/local_object_storage/writecache/flush_test.go
@ -316,6 +317,15 @@ func newObject(t *testing.T, size int) (*object.Object, []byte) {
return obj, data
fyrchik (Migrated from github.com) commented 2023-02-16 06:08:51 +00:00
Copy link

Is it enough with -race flag?

Is it enough with `-race` flag?
fyrchik (Migrated from github.com) commented 2023-02-16 06:14:47 +00:00
Copy link

The tests are in the writecache package, can we just directly check for initialized flag?

The tests are in the `writecache` package, can we just directly check for `initialized` flag?
pkg/local_object_storage/writecache/init.go
@ -16,0 +41,4 @@
select {
case <-c.stopInitCh:
return
case <-c.closeCh:
fyrchik (Migrated from github.com) commented 2023-02-16 06:10:10 +00:00
Copy link

If we use c.wg here, do we need to use it in 2 previous goroutines?

If we use `c.wg` here, do we need to use it in 2 previous goroutines?
pkg/local_object_storage/writecache/init.go
@ -20,2 +66,4 @@
}
flushed, needRemove := c.flushStatus(addr)
if flushed {
fyrchik (Migrated from github.com) commented 2023-02-16 06:11:02 +00:00
Copy link

It is enough for setMode to work too?

It is enough for `setMode` to work too?
pkg/local_object_storage/writecache/mode.go
fyrchik (Migrated from github.com) commented 2023-02-16 06:12:25 +00:00
Copy link

I would add yet, not to spread panic among testers, administrators and service engineers.

I would add ` yet`, not to spread panic among testers, administrators and service engineers.
pkg/local_object_storage/writecache/writecache.go
@ -154,3 +162,3 @@
// Finish all in-progress operations.
if err := c.SetMode(mode.ReadOnly); err != nil {
if err := c.setMode(mode.ReadOnly); err != nil {
return err
fyrchik (Migrated from github.com) commented 2023-02-16 06:13:19 +00:00
Copy link

Is this line necessary?

Is this line necessary?
carpawell (Migrated from github.com) reviewed 2023-02-17 14:00:14 +00:00
pkg/local_object_storage/writecache/writecache.go
@ -154,3 +162,3 @@
// Finish all in-progress operations.
if err := c.SetMode(mode.ReadOnly); err != nil {
if err := c.setMode(mode.ReadOnly); err != nil {
return err
carpawell (Migrated from github.com) commented 2023-02-17 14:00:13 +00:00
Copy link

you mean the empty one?

you mean the empty one?
carpawell (Migrated from github.com) reviewed 2023-02-17 14:00:16 +00:00
pkg/local_object_storage/writecache/init.go
@ -20,2 +66,4 @@
}
flushed, needRemove := c.flushStatus(addr)
if flushed {
carpawell (Migrated from github.com) commented 2023-02-17 14:00:15 +00:00
Copy link

was not, fixed

was not, fixed
carpawell (Migrated from github.com) reviewed 2023-02-17 14:00:19 +00:00
pkg/local_object_storage/writecache/flush_test.go
@ -316,6 +317,15 @@ func newObject(t *testing.T, size int) (*object.Object, []byte) {
return obj, data
carpawell (Migrated from github.com) commented 2023-02-17 14:00:18 +00:00
Copy link

it was not but because of another reason, fixed races

it was not but because of another reason, fixed races
carpawell commented 2023-02-17 14:01:34 +00:00 (Migrated from github.com)
Copy link

@fyrchik, fixed tests with -race flag but now PR is a little bit more complex. Review one more time, please. Now it works but I hope it will be refactored in the future anyway.

@fyrchik, fixed tests with `-race` flag but now PR is a little bit more complex. Review one more time, please. Now it works but I hope it will be refactored in the future anyway.
acid-ant (Migrated from github.com) reviewed 2023-02-17 14:21:45 +00:00
pkg/local_object_storage/writecache/generic_test.go
acid-ant (Migrated from github.com) commented 2023-02-17 14:16:01 +00:00
Copy link

Looks like needs to be deleted.

Looks like needs to be deleted.
carpawell (Migrated from github.com) reviewed 2023-02-17 14:30:47 +00:00
pkg/local_object_storage/writecache/generic_test.go
carpawell (Migrated from github.com) commented 2023-02-17 14:30:47 +00:00
Copy link

wow, dropped

wow, dropped
acid-ant (Migrated from github.com) reviewed 2023-02-17 14:34:52 +00:00
fyrchik (Migrated from github.com) reviewed 2023-02-21 07:07:45 +00:00
pkg/local_object_storage/writecache/flush_test.go
fyrchik (Migrated from github.com) commented 2023-02-20 11:29:07 +00:00
Copy link

So, basically rawWc.initialized.Load?

So, basically `rawWc.initialized.Load`?
fyrchik (Migrated from github.com) commented 2023-02-20 11:29:25 +00:00
Copy link

Why do you use a mutex with atomic initialized?

Why do you use a mutex with atomic `initialized`?
pkg/local_object_storage/writecache/mode.go
fyrchik (Migrated from github.com) commented 2023-02-20 11:34:48 +00:00
Copy link

Why have you decided to use defer here?

What happens if we fail to Close/Open db? If we move to a degraded mode?

Why have you decided to use defer here? What happens if we fail to `Close`/`Open` db? If we move to a degraded mode?
pkg/local_object_storage/writecache/mode.go
@ -28,0 +42,4 @@
c.initWG.Wait()
c.stopInitCh = make(chan struct{})
defer func() {
fyrchik (Migrated from github.com) commented 2023-02-20 11:36:10 +00:00
Copy link

The order is really important, can you mention that stopInitCh must be used only in initWG goroutines, to avoid races?

The order is really important, can you mention that `stopInitCh` must be used only in `initWG` goroutines, to avoid races?
carpawell (Migrated from github.com) reviewed 2023-02-28 20:05:09 +00:00
pkg/local_object_storage/writecache/flush_test.go
carpawell (Migrated from github.com) commented 2023-02-28 20:05:08 +00:00
Copy link

it was not an atomic before and then i just changed it and forget other cases to be adopted too

fixed

it was not an `atomic` before and then i just changed it and forget other cases to be adopted too fixed
carpawell (Migrated from github.com) reviewed 2023-02-28 20:05:10 +00:00
pkg/local_object_storage/writecache/mode.go
carpawell (Migrated from github.com) commented 2023-02-28 20:05:10 +00:00
Copy link

flashbacks from the previous implementation

looked at that one more time and it seems like an error-free and non-degraded mode switch is enough for running initFlushMarks one more time? check the updated version, please

flashbacks from the previous implementation looked at that one more time and it seems like an error-free and non-degraded mode switch is enough for running `initFlushMarks` one more time? check the updated version, please
carpawell (Migrated from github.com) reviewed 2023-02-28 20:05:11 +00:00
pkg/local_object_storage/writecache/mode.go
@ -28,0 +42,4 @@
c.initWG.Wait()
c.stopInitCh = make(chan struct{})
defer func() {
carpawell (Migrated from github.com) commented 2023-02-28 20:05:11 +00:00
Copy link

sure, added comments to that fields

sure, added comments to that fields
acid-ant (Migrated from github.com) approved these changes 2023-03-01 06:24:35 +00:00
dstepanov-yadro (Migrated from github.com) approved these changes 2023-03-01 09:03:56 +00:00
fyrchik added a new dependency 2023-03-06 19:19:51 +00:00
#32 Initialize write-cache asynchronously
fyrchik removed a dependency 2023-03-06 19:21:14 +00:00
#32 Initialize write-cache asynchronously
fyrchik approved these changes 2023-03-09 11:07:24 +00:00
fyrchik merged commit f1f3c80dbf into master 2023-03-09 11:07:34 +00:00
fyrchik deleted branch carpawell/faster-wc-init 2023-03-09 11:07:35 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
fyrchik
ale64bit
acid-ant
dstepanov-yadro
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#35
No description provided.
Delete branch "carpawell/faster-wc-init"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?