TrueCloudLab/frostfs-qos
14
0
Fork 6
Code Issues 1 Pull requests Projects Releases Packages Activity Actions

Allow to restrict tag requests #15

Merged
dstepanov-yadro merged 2 commits from dstepanov-yadro/frostfs-qos:feat/allow_to_deny_tag_requests into master 2025-03-31 08:08:14 +00:00
Conversation 3 Commits 2 Files changed 3 +25 -1
dstepanov-yadro commented 2025-03-28 10:42:29 +00:00
Member
Copy link

It is now possible to restrict requests for a specific tag.
A separate field in TagInfo is used to avoid comparing float64 values with zero.

Closes #14

It is now possible to restrict requests for a specific tag. A separate field in `TagInfo` is used to avoid comparing float64 values with zero. Closes #14
dstepanov-yadro added 1 commit 2025-03-28 10:42:29 +00:00
[#14] mclock: Allow to restrict tag requests
All checks were successful
DCO action / DCO (pull_request) Successful in 23s
Details
Vulncheck / Vulncheck (pull_request) Successful in 39s
Details
Tests and linters / Lint (pull_request) Successful in 1m12s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m14s
Details
Tests and linters / Run gofumpt (pull_request) Successful in 1m5s
Details
Tests and linters / Staticcheck (pull_request) Successful in 1m13s
Details
Tests and linters / gopls check (pull_request) Successful in 1m11s
Details
Tests and linters / Tests (pull_request) Successful in 1m21s
Details
Tests and linters / Tests with -race (pull_request) Successful in 1m34s
Details
b9aae451c2 
It is now possible to restrict requests for a specific tag.
A separate field in `TagInfo` is used to avoid comparing float64 values with zero.

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
requested reviews from fyrchik, storage-core-committers, storage-core-developers 2025-03-28 10:42:29 +00:00
dstepanov-yadro added 1 commit 2025-03-28 10:43:51 +00:00
[#14] CODEOWNERS: Use core commiters and developers groups
All checks were successful
DCO action / DCO (pull_request) Successful in 27s
Details
Vulncheck / Vulncheck (pull_request) Successful in 35s
Details
Tests and linters / Staticcheck (pull_request) Successful in 1m3s
Details
Tests and linters / Run gofumpt (pull_request) Successful in 1m1s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m12s
Details
Tests and linters / Lint (pull_request) Successful in 1m22s
Details
Tests and linters / Tests (pull_request) Successful in 1m26s
Details
Tests and linters / Tests with -race (pull_request) Successful in 1m25s
Details
Tests and linters / gopls check (pull_request) Successful in 1m26s
Details
3b39023d45 
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
fyrchik reviewed 2025-03-28 10:57:36 +00:00
fyrchik left a comment
Owner
Copy link

  1. Regarding the commit message: IOPS is integral, so we could compare float64 with < 0.5?
    What happens if this new flag contradicts others? The caller should still somehow convert numbers to bool, right, so why not do it here instead?
    We can still have integers in public interface, but use float64 internally.

  2. Imagine we have 0.0 in all fields, what would be the behaviour of the current code?

  3. TagInfo is provided once on queue construction? If so, it seems this change allows us only to return more precise error, or am I missing something?

1. Regarding the commit message: `IOPS` is integral, so we could compare `float64` with `< 0.5`? What happens if this new flag contradicts others? The caller should still somehow convert numbers to bool, right, so why not do it here instead? We can still have integers in public interface, but use float64 internally. 2. Imagine we have `0.0` in all fields, what would be the behaviour of the current code? 3. TagInfo is provided once on queue construction? If so, it seems this change allows us only to return more precise error, or am I missing something?
dstepanov-yadro commented 2025-03-28 11:20:53 +00:00
Author
Member
Copy link

@fyrchik wrote in #15 (comment):

  1. Regarding the commit message: IOPS is integral, so we could compare float64 with < 0.5?
    What happens if this new flag contradicts others? The caller should still somehow convert numbers to bool, right, so why not do it here instead?
    We can still have integers in public interface, but use float64 internally.

    1. Imagine we have 0.0 in all fields, what would be the behaviour of the current code?

    2. TagInfo is provided once on queue construction? If so, it seems this change allows us only to return more precise error, or am I missing something?

  1. I think that having separate field allows us to avoid compare float64 against 0. With raw integers it is not possible to set limit to 0.5 requests per second. It is still possible to use uint with some implicit multiplier, but I don't think it will be simpler than raw float64 values.
  2. 0.0 values are prohibited by parameters validation.
  3. ErrTagRequestsRestricted error is returned now, this error returns only in case when tag operation are restricted.
@fyrchik wrote in https://git.frostfs.info/TrueCloudLab/frostfs-qos/pulls/15#issuecomment-71662: > 1. Regarding the commit message: `IOPS` is integral, so we could compare `float64` with `< 0.5`? > What happens if this new flag contradicts others? The caller should still somehow convert numbers to bool, right, so why not do it here instead? > We can still have integers in public interface, but use float64 internally. > > 2. Imagine we have `0.0` in all fields, what would be the behaviour of the current code? > > 3. TagInfo is provided once on queue construction? If so, it seems this change allows us only to return more precise error, or am I missing something? 1. I think that having separate field allows us to avoid compare `float64` against 0. With raw integers it is not possible to set limit to 0.5 requests per second. It is still possible to use uint with some implicit multiplier, but I don't think it will be simpler than raw float64 values. 2. 0.0 values are prohibited by parameters validation. 3. `ErrTagRequestsRestricted` error is returned now, this error returns only in case when tag operation are restricted.
fyrchik approved these changes 2025-03-31 07:58:27 +00:00
dstepanov-yadro force-pushed feat/allow_to_deny_tag_requests from 3b39023d45 to b5ed0b6eff 2025-03-31 08:04:41 +00:00 Compare
dstepanov-yadro merged commit b5ed0b6eff into master 2025-03-31 08:08:14 +00:00
dstepanov-yadro deleted branch feat/allow_to_deny_tag_requests 2025-03-31 08:08:18 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
Labels
Clear labels
  
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
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-qos#15
No description provided.
Delete branch "dstepanov-yadro/frostfs-qos:feat/allow_to_deny_tag_requests"

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?