TrueCloudLab/xk6-frostfs
42
1
Fork 20
Code Issues 33 Pull requests 1 Releases Activity Actions

registry: Try to continue iteration #131

Merged
dstepanov-yadro merged 3 commits from dstepanov-yadro/xk6-frostfs:fix/registry_selector into master 2024-09-04 19:51:18 +00:00
Conversation 1 Commits 3 Files changed 12 +1056 -999
dstepanov-yadro commented 2024-03-19 14:21:08 +00:00
Member
Copy link

If selector has read all the objects then it tries first to continue iteration.

If selector has read all the objects then it tries first to continue iteration.
dstepanov-yadro force-pushed fix/registry_selector from 4502f3ddfc to 7db213f352 2024-03-19 14:22:02 +00:00 Compare
dstepanov-yadro force-pushed fix/registry_selector from 7db213f352 to cc90ad6070 2024-03-19 14:23:41 +00:00 Compare
dstepanov-yadro force-pushed fix/registry_selector from cc90ad6070 to 5745dba1fc 2024-03-19 14:25:17 +00:00 Compare
fyrchik reviewed 2024-03-19 14:28:17 +00:00
internal/registry/obj_selector.go Outdated
@ -167,7 +170,6 @@ func (o *ObjSelector) selectLoop() {
case <-o.ctx.Done():
return
}
lastID = 0
fyrchik commented 2024-03-19 14:28:16 +00:00
Owner
Copy link

With this change alone, do we still have duplicates? It seems the culprit

It was introduced in 3c26e7c917 without justification in the commit message (which seemed to be a part of #86, but I cannot see the commit there).

With this change alone, do we still have duplicates? It seems the culprit It was introduced in 3c26e7c9175 without justification in the commit message (which seemed to be a part of #86, but I cannot see the commit there).
dstepanov-yadro commented 2024-03-19 14:33:44 +00:00
Author
Member
Copy link

With this change alone selector will read registry file only once. This affects read scenarios.

With this change alone selector will read registry file only once. This affects read scenarios.
fyrchik commented 2024-03-19 14:36:23 +00:00
Owner
Copy link

Can we made an explicit opt-in/opt-out for this behaviour? It seems that in each scenario we want a specific semantics (for read -- read many times, for delete -- read once).

Can we made an explicit opt-in/opt-out for this behaviour? It seems that in each scenario we want a specific semantics (for read -- read many times, for delete -- read once).
fyrchik commented 2024-03-19 14:36:53 +00:00
Owner
Copy link

To me "read-once" by default seems expected.

To me "read-once" by default seems expected.
dstepanov-yadro commented 2024-03-19 14:43:37 +00:00
Author
Member
Copy link

But for write + update + delete read once doesn't work too.
There have been no other comments on this behavior in 8 months. That's why I think it's not necessary to complicate things.

But for write + update + delete `read once` doesn't work too. There have been no other comments on this behavior in 8 months. That's why I think it's not necessary to complicate things.
fyrchik commented 2024-03-19 14:46:44 +00:00
Owner
Copy link

For write + update + delete we use a different selector for each step.
I don't see how it complicates things, other than making them more explicit.
The default is up to you, but each time you create a selector you probably want one specific behaviour, not a random combination of two.

For write + update + delete we use a different selector for each step. I don't see how it complicates things, other than making them more explicit. The default is up to you, but each time you create a selector you probably want one specific behaviour, not a random combination of two.
fyrchik commented 2024-03-19 14:48:24 +00:00
Owner
Copy link

It also doesn't need to be a parameter, maybe 2 different constructors.

It also doesn't need to be a parameter, maybe 2 different constructors.
dstepanov-yadro commented 2024-03-19 15:07:31 +00:00
Author
Member
Copy link

Now selector is simple: it just reads objects in a circle. This works fine with the most common scenarios: write+read and read.
I have not received any read once-behavior requests from QA. So I don't think it is really required.

Now selector is simple: it just reads objects in a circle. This works fine with the most common scenarios: write+read and read. I have not received any `read once`-behavior requests from QA. So I don't think it is really required.
fyrchik commented 2024-03-19 15:14:17 +00:00
Owner
Copy link

The bug is exactly about the cycling behaviour: we do not need it if we want to remove each object exactly once.

The bug is exactly about the cycling behaviour: we do not need it if we want to remove each object exactly once.
dstepanov-yadro commented 2024-03-19 15:32:04 +00:00
Author
Member
Copy link

The scenario is write+update+delete - 10 workers put objects, 10 workers update those objects, 10 workers delete updated objects. So with read-once update and delete workers will hang stop after first iteration.

The scenario is `write+update+delete` - 10 workers put objects, 10 workers update those objects, 10 workers delete updated objects. So with `read-once` update and delete workers will ~~hang~~ stop after first iteration.
fyrchik commented 2024-03-20 05:31:38 +00:00
Owner
Copy link

No, because registry groups objects by status.
So we have update use read once object with status "created", then put them in registry with status "updated"
delete will read once object with status "updated", then put then in registry with status "deleted".

The behaviour was there before the 3c26e7c917 , where a bug was introduced to support read scenarios. A proper solution would've been to do exactly what we need: introduce cycling behaviour for selectors only for read scenarios.

No, because registry groups objects by status. So we have `update` use `read once object with status "created", then put them in registry with status "updated"` `delete` will `read once object with status "updated", then put then in registry with status "deleted"`. The behaviour was there before the 3c26e7c917 , where a bug was introduced to support read scenarios. A proper solution would've been to do exactly what we need: introduce cycling behaviour for selectors only for read scenarios.
dstepanov-yadro commented 2024-03-20 06:11:19 +00:00
Author
Member
Copy link

Then I don't understand what you mean by read-once :(
Current implementation always tries to continue iteration first. If there is no new items, it starts from the beginning. This allows to work with update and read scenarios without any additional flags.

Then I don't understand what you mean by `read-once` :( Current implementation always tries to continue iteration first. If there is no new items, it starts from the beginning. This allows to work with update and read scenarios without any additional flags.
dstepanov-yadro commented 2024-03-20 08:07:50 +00:00
Author
Member
Copy link

done

done
fyrchik marked this conversation as resolved
dstepanov-yadro requested review from storage-core-committers 2024-03-19 14:44:21 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-03-19 14:44:21 +00:00
dstepanov-yadro force-pushed fix/registry_selector from 5745dba1fc to 6350c88c9e 2024-03-20 08:05:28 +00:00 Compare
fyrchik reviewed 2024-03-20 08:17:56 +00:00
fyrchik left a comment
Owner
Copy link

After reformatting js code may be add clang-format to fmt target?

After reformatting js code may be add clang-format to fmt target?
internal/registry/obj_selector.go Outdated
@ -125,6 +127,9 @@ func (o *ObjSelector) selectLoop() {
if keyBytes != nil && decodeId(keyBytes) == lastID {
keyBytes, objBytes = c.Next()
}
if o.looped && keyBytes == nil { // if no more items added then try to start from the beginning
fyrchik commented 2024-03-20 08:16:49 +00:00
Owner
Copy link

After looped addition, do we still need this change? It seems lastID=0 did the same thing.

After `looped` addition, do we still need this change? It seems `lastID=0` did the same thing.
dstepanov-yadro commented 2024-03-20 08:55:09 +00:00
Author
Member
Copy link

done

done
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed fix/registry_selector from 6350c88c9e to 0a6e51ccc9 2024-03-20 08:42:08 +00:00 Compare
dstepanov-yadro commented 2024-03-20 08:54:19 +00:00
Author
Member
Copy link

After reformatting js code may be add clang-format to fmt target?

I use clang-format-17, someone else may have different version. Anyway this js-files changed not so frequently I think.

> After reformatting js code may be add clang-format to fmt target? I use clang-format-17, someone else may have different version. Anyway this js-files changed not so frequently I think.
fyrchik approved these changes 2024-03-20 09:22:42 +00:00
aarifullin approved these changes 2024-03-20 10:12:45 +00:00
dstepanov-yadro merged commit 0a6e51ccc9 into master 2024-03-20 10:13:24 +00:00
dstepanov-yadro deleted branch fix/registry_selector 2024-03-20 10:13:25 +00:00
dstepanov-yadro referenced this pull request from a commit 2024-03-20 10:13:26 +00:00
[#131] registry: Gofumpt
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
aarifullin
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
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
good first issue
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
3 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/xk6-frostfs#131
No description provided.
Delete branch "dstepanov-yadro/xk6-frostfs:fix/registry_selector"

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?