TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 5 Projects Releases 1 Activity Actions

writecache: Remove usage of close channel in bbolt and badger #688

Merged
fyrchik merged 2 commits from acid-ant/frostfs-node:feature/642-wc-flush-with-ctx-from-main into master 2023-10-24 15:57:52 +00:00
Conversation 0 Commits 2 Files changed 5 +49 -65
acid-ant commented 2023-09-13 17:00:02 +00:00
Member
Copy link

Close #642

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #642 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-09-13 17:00:13 +00:00
acid-ant requested review from storage-core-developers 2023-09-13 17:00:14 +00:00
fyrchik reviewed 2023-09-14 08:15:39 +00:00
pkg/local_object_storage/writecache/writecachebadger/cachebadger.go Outdated
@ -104,2 +99,2 @@
c.runFlushLoop()
c.runGCLoop()
ctx, cancel := context.WithCancel(context.Background())
c.cancel = cancel
fyrchik commented 2023-09-14 08:10:47 +00:00
Owner
Copy link

Not protected by modeMtx here as the comment says.

Not protected by `modeMtx` here as the comment says.
👍 1
acid-ant commented 2023-09-14 12:51:08 +00:00
Author
Member
Copy link

Protected in Close, as it was for closeCh too. Thought this comment is about this.

Protected in `Close`, as it was for `closeCh` too. Thought this comment is about this.
fyrchik commented 2023-09-14 14:01:11 +00:00
Owner
Copy link

Ok, it seems closeCh had this problem too

Ok, it seems `closeCh` had this problem too
fyrchik marked this conversation as resolved
pkg/local_object_storage/writecache/writecachebadger/cachebadger.go Outdated
@ -123,3 +120,3 @@
defer c.modeMtx.Unlock()
c.closeCh = nil
c.cancel = nil
fyrchik commented 2023-09-14 08:11:08 +00:00
Owner
Copy link

What about setting it to nil right after we called it?

What about setting it to nil right after we called it?
acid-ant commented 2023-09-14 13:26:15 +00:00
Author
Member
Copy link

Good catch, updated.

Good catch, updated.
fyrchik marked this conversation as resolved
pkg/local_object_storage/writecache/writecachebadger/flush.go Outdated
@ -36,21 +36,18 @@ const (
)
type collector struct {
cache *cache
fyrchik commented 2023-09-14 08:14:26 +00:00
Owner
Copy link

Why this change?

Why this change?
aarifullin commented 2023-09-14 08:18:31 +00:00
Member
Copy link

Agree. I also don't understand the intention to remove these fields and pass them to send method. Could you explain, please?

Agree. I also don't understand the intention to remove these fields and pass them to `send` method. Could you explain, please?
acid-ant commented 2023-09-14 12:29:59 +00:00
Author
Member
Copy link

That was the previous implementation when I tried to use context from main. Also, the idea was to reduce the amount of entities. Let's revert these changes.

That was the previous implementation when I tried to use context from main. Also, the idea was to reduce the amount of entities. Let's revert these changes.
👍 1
pkg/local_object_storage/writecache/writecachebadger/flush.go Outdated
@ -149,2 +142,3 @@
// All calls to Send are done by a single goroutine
stream.Send = coll.Send
stream.Send = func(buf *z.Buffer) error {
return coll.send(ctx, cancel, c, buf)
fyrchik commented 2023-09-14 08:14:00 +00:00
Owner
Copy link

Do we need this cancel argument, why not cancel context after Orchestrate()?

Do we need this `cancel` argument, why not cancel context after `Orchestrate()`?
acid-ant commented 2023-09-14 12:42:30 +00:00
Author
Member
Copy link

We need to interrupt at the moment when scheduled more than flushBatchSize items for flush. That is why I use cancel inside Send.

We need to interrupt at the moment when scheduled more than `flushBatchSize` items for flush. That is why I use `cancel` inside `Send`.
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from 699b6a7b24 to fbfff9ef3f 2023-09-14 13:25:58 +00:00 Compare
fyrchik approved these changes 2023-09-15 14:39:59 +00:00
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from fbfff9ef3f to ec46096a80 2023-09-18 08:58:57 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from ec46096a80 to 82b2bf0c4d 2023-09-18 10:53:12 +00:00 Compare
dstepanov-yadro requested changes 2023-09-18 12:53:31 +00:00
pkg/local_object_storage/writecache/writecachebbolt/cachebbolt.go Outdated
@ -32,3 +32,2 @@
flushCh chan *objectSDK.Object
// closeCh is close channel, protected by modeMtx.
closeCh chan struct{}
// cancel is cancel function, protected by modeMtx.
dstepanov-yadro commented 2023-09-18 12:52:50 +00:00
Member
Copy link

Init is not protected by modeMtx. Comment or behaviour should be fixed.

`Init` is not protected by modeMtx. Comment or behaviour should be fixed.
acid-ant commented 2023-09-19 05:48:34 +00:00
Author
Member
Copy link

Description updated.

Description updated.
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from 82b2bf0c4d to 1c6b72c7a1 2023-09-19 05:47:04 +00:00 Compare
acid-ant requested review from dstepanov-yadro 2023-09-19 06:07:29 +00:00
dstepanov-yadro approved these changes 2023-09-20 14:01:09 +00:00
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from 1c6b72c7a1 to e306ee1b0c 2023-09-28 13:38:32 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from e306ee1b0c to b5df54f407 2023-10-03 11:18:09 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from 4f5f4c92f2 to e927399d50 2023-10-12 07:01:29 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from e927399d50 to dae60f7dd9 2023-10-16 07:19:24 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from dae60f7dd9 to edb5be5ec8 2023-10-17 10:29:57 +00:00 Compare
acid-ant force-pushed feature/642-wc-flush-with-ctx-from-main from edb5be5ec8 to 0f4633d65d 2023-10-17 10:33:40 +00:00 Compare
fyrchik merged commit 559ad58ab1 into master 2023-10-24 15:57:52 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
dstepanov-yadro
TrueCloudLab/storage-core-developers
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
4 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#688
No description provided.
Delete branch "acid-ant/frostfs-node:feature/642-wc-flush-with-ctx-from-main"

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?