TrueCloudLab
/
xk6-frostfs
36
0
Fork 14
Code Issues 30 Pull Requests Actions Releases Activity

registry: Try to continue iteration #131

Merged
dstepanov-yadro merged 3 commits from dstepanov-yadro/xk6-frostfs:fix/registry_selector into master 2024-03-20 10:13:24 +00:00
Conversation 1 Commits 3 Files Changed 12
dstepanov-yadro commented 2024-03-19 14:21:08 +00:00
Collaborator

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 added 1 commit 2024-03-19 14:21:10 +00:00
DCO action / DCO (pull_request) Successful in 55s Details
Tests and linters / Tests (1.21) (pull_request) Successful in 3m27s Details
Tests and linters / Tests (1.20) (pull_request) Successful in 4m18s Details
Tests and linters / Tests with -race (pull_request) Successful in 4m16s Details
Tests and linters / Lint (pull_request) Failing after 5m35s Details
4502f3ddfc [#9999] registry: Try to continue iteration 
If selector has read all the objects then it tries first to continue
iteration.

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
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

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
Poster
Collaborator

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

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

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
Poster
Collaborator

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

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

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
Poster
Collaborator

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

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
Poster
Collaborator

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

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
Poster
Collaborator

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
Poster
Collaborator

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

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

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
Poster
Collaborator

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
Poster
Collaborator

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
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 Label
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
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
There is no content yet.
Delete Branch "dstepanov-yadro/xk6-frostfs:fix/registry_selector"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?