TrueCloudLab/frostfs-sdk-go
30
0
Fork 21
Code Issues 30 Pull requests 3 Releases Activity Actions

netmap: Introduce Replica method #211

Merged
fyrchik merged 1 commit from aarifullin/frostfs-sdk-go:fix/ec_total_count into master 2024-09-04 19:51:15 +00:00
Conversation 0 Commits 1 Files changed 2 +11 -2
aarifullin commented 2024-03-25 21:03:17 +00:00
Member
Copy link

* Logically, ECShardsCount is mutually exclusive with ReplicaNumberByIndex.
* ECShardsCount returns non-zero results if a policy is set by EC and zero if it is set by REP. Thus ECShardsCount and ReplicaNumberByIndex can be used to distinguish policies.
* Add explanatory comments for the methods.

  • Make ReplicaNumberByIndex deprecated.
  • Introduce Replica method that access i-th replica directly. This allows the direct access GetCount() and GetECDataCount, GetECParityCount.
  • Also, fix setIDWithSignature (we calculate checksum before ID is calculated)
~~* Logically, ECShardsCount is mutually exclusive with ReplicaNumberByIndex.~~ ~~* ECShardsCount returns non-zero results if a policy is set by `EC` and zero if it is set by `REP`. Thus ECShardsCount and ReplicaNumberByIndex can be used to distinguish policies.~~ ~~* Add explanatory comments for the methods.~~ * Make ReplicaNumberByIndex deprecated. * Introduce Replica method that access i-th replica directly. This allows the direct access `GetCount()` and `GetECDataCount`, `GetECParityCount`. * Also, fix `setIDWithSignature` (we calculate checksum before ID is calculated)
aarifullin requested review from storage-core-committers 2024-03-25 21:03:28 +00:00
aarifullin requested review from storage-core-developers 2024-03-25 21:03:29 +00:00
aarifullin force-pushed fix/ec_total_count from 2b712ac16f to 38a239fd15 2024-03-25 21:03:59 +00:00 Compare
fyrchik reviewed 2024-03-26 07:31:17 +00:00
netmap/policy.go Outdated
@ -185,1 +185,4 @@
// ECTotalCountByIndex returns the total count of data and parity parts used by EC
// from the i-th replica descriptor. Index MUST be in range [0; NumberOfReplicas()).
func (p PlacementPolicy) ECTotalCountByIndex(i int) uint32 {
fyrchik commented 2024-03-26 07:31:14 +00:00
Owner
Copy link

The interface for EC is intentionally minimal, we haven't even written in node anything yet, why this PR?

The interface for EC is intentionally minimal, we haven't even written in node anything yet, why this PR?
aarifullin commented 2024-03-26 08:33:09 +00:00
Author
Member
Copy link

Firstly, I thought we need to distinguish how we set replicas[i] - either by REP or by EC. Anyway, even the introduced function seems inconvenient, we need to fix ReplicaNumberByIndex then - it should use countNodes(p.replicas[i]) instread, because it retruns 0 for EC policy right now

Firstly, I thought we need to distinguish how we set `replicas[i]` - either by `REP` or by `EC`. Anyway, even the introduced function seems inconvenient, we need to fix `ReplicaNumberByIndex` then - it should use `countNodes(p.replicas[i])` instread, because it retruns `0` for EC policy right now
aarifullin commented 2024-03-26 08:38:42 +00:00
Author
Member
Copy link

I have removed this method and fixed ReplicaNumberByIndex.
Commit message is also fixed. Please, check this out. We need this fix because we need to get EC parts total count. But still here is a problem how we can distinguish a replica is set - by REP or by EC?

I have removed this method and fixed `ReplicaNumberByIndex`. Commit message is also fixed. Please, check this out. We need this fix because we need to get `EC` parts total count. But still here is a problem how we can distinguish a replica is set - by `REP` or by `EC`?
fyrchik commented 2024-03-26 09:50:10 +00:00
Owner
Copy link

Oh, I see what you did.
So it is motivated by internal SDK needs?
Doesn't node selection work right now? Probably missed the test in my initial PR.

Anyway, what about ECShardsCount (uint32, uint32) then? It seems if we need total amount from a public method, we also need data and parity?

Oh, I see what you did. So it is motivated by internal SDK needs? Doesn't node selection work right now? Probably missed the test in my initial PR. Anyway, what about `ECShardsCount (uint32, uint32)` then? It seems if we need total amount from a public method, we also need data and parity?
aarifullin commented 2024-03-26 10:48:38 +00:00
Author
Member
Copy link

This sounds goods. Anyway, as I said, we need to distinguish the approach and thus ECShardsCount seems OK for that

This sounds goods. Anyway, as I said, we need to distinguish the approach and thus `ECShardsCount` seems OK for that
aarifullin force-pushed fix/ec_total_count from 38a239fd15 to 5007db4579 2024-03-26 08:36:11 +00:00 Compare
aarifullin force-pushed fix/ec_total_count from 5007db4579 to 56e75743ec 2024-03-26 08:38:22 +00:00 Compare
aarifullin changed title from netmap: Introduce ECTotalCountByIndex method to netmap: Fix ReplicaNumberByIndex method 2024-03-26 08:39:11 +00:00
fyrchik reviewed 2024-03-26 12:04:51 +00:00
netmap/policy.go Outdated
@ -181,3 +181,3 @@
// Zero PlacementPolicy has no replicas.
func (p PlacementPolicy) ReplicaNumberByIndex(i int) uint32 {
return p.replicas[i].GetCount()
return countNodes(p.replicas[i])
fyrchik commented 2024-03-26 12:04:51 +00:00
Owner
Copy link

Comment needs to be adjusted too.
How do we distinguish between REP and EC policies in existing places?

Comment needs to be adjusted too. How do we distinguish between REP and EC policies in existing places?
aarifullin commented 2024-03-27 11:35:52 +00:00
Author
Member
Copy link

You suggested the idea I liked - ECShardsCount (uint32, uint32). So, ReplicaNumberByIndex will be used for REP polices and ECShardsCount - for EC

You suggested the idea I liked - `ECShardsCount (uint32, uint32)`. So, `ReplicaNumberByIndex` will be used for `REP` polices and `ECShardsCount` - for `EC`
aarifullin force-pushed fix/ec_total_count from 56e75743ec to b42bee3454 2024-03-27 11:46:59 +00:00 Compare
aarifullin reviewed 2024-03-27 11:47:31 +00:00
go.mod Outdated
@ -2,6 +2,8 @@ module git.frostfs.info/TrueCloudLab/frostfs-sdk-go
go 1.20
replace git.frostfs.info/TrueCloudLab/frostfs-api-go/v2 v2.16.1-0.20240319122301-1772b921826b => git.frostfs.info/aarifullin/frostfs-api-go/v2 v2.15.1-0.20240327095603-491a47e7fe24
aarifullin commented 2024-03-27 11:47:31 +00:00
Author
Member
Copy link

This will be fixed as soon as #70 is merged

This will be fixed as soon as [#70](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/pulls/70/files) is merged
fyrchik commented 2024-03-28 08:14:42 +00:00
Owner
Copy link

It is merged.

It is merged.
aarifullin marked this conversation as resolved
aarifullin force-pushed fix/ec_total_count from b42bee3454 to 9536991d7d 2024-03-27 11:48:21 +00:00 Compare
aarifullin force-pushed fix/ec_total_count from 9536991d7d to fb4a31d03e 2024-03-27 11:49:14 +00:00 Compare
aarifullin force-pushed fix/ec_total_count from fb4a31d03e to b59dac7aee 2024-03-27 11:50:22 +00:00 Compare
aarifullin changed title from netmap: Fix ReplicaNumberByIndex method to netmap: Introduce ECShardsCount method 2024-03-27 12:06:38 +00:00
fyrchik reviewed 2024-03-28 08:18:50 +00:00
netmap/policy.go Outdated
@ -182,1 +182,4 @@
//
// If the result is zero, then EC policy might be used - use ECShardsCountByIndex
// to check that.
func (p PlacementPolicy) ReplicaNumberByIndex(i int) uint32 {
fyrchik commented 2024-03-28 08:18:48 +00:00
Owner
Copy link

I am not wondering:
Consider policer. It could happily remove the replica because ReplicaNumberByIndex returns 0. Not saying it will happen, but looks like a probable mistake. In general it seems dangerous for automatic processing. However, returning a non-zero number here is also not perfect.

How about returning replica with Replica(i int) Replicaor may be TypeOf(i int) and then switch based on type?
We can panic if ReplicaNumberByIndex is used for EC then.
Another option is to add , bool) to the return value which signifies success.

I am not wondering: Consider policer. It _could_ happily remove the replica because `ReplicaNumberByIndex` returns 0. Not saying it will happen, but looks like a probable mistake. In general it seems dangerous for automatic processing. However, returning a non-zero number here is also not perfect. How about returning replica with `Replica(i int) Replica`or may be `TypeOf(i int)` and then switch based on type? We can panic if `ReplicaNumberByIndex` is used for EC then. Another option is to add `, bool)` to the return value which signifies success.
dstepanov-yadro commented 2024-03-28 10:58:22 +00:00
Member
Copy link

Consider policer.

If policer uses old SDK version, then ReplicaNumberByIndex returns 0 for EC anyway.
If policer uses new (with EC) SDK version, then policer must know how to deal with EC.

> Consider policer. If policer uses old SDK version, then `ReplicaNumberByIndex` returns 0 for EC anyway. If policer uses new (with EC) SDK version, then policer must know how to deal with EC.
aarifullin commented 2024-03-28 11:51:18 +00:00
Author
Member
Copy link

then policer must know how to deal with EC.

That's the good point. Anyway, we cannot distinguish how Replica was set - either by REP or by EC because there is no indicating flag. The only way is to use these numbers (approaches suggested by @fyrchik will use counting anyway): if ReplicaNumberByIndex > 0, then REP is used; if ECShardsCount > 0, then EC is used; if they are both zero, then the policer can consider that Replica is REP 0.

> then policer must know how to deal with EC. That's the good point. Anyway, we cannot distinguish how `Replica` was set - either by `REP` or by `EC` because there is no indicating flag. **The only way is to use these numbers** (approaches suggested by @fyrchik will use counting *anyway*): if `ReplicaNumberByIndex > 0`, then `REP` is used; if `ECShardsCount > 0`, then `EC` is used; if they are both zero, then the policer can consider that `Replica` is `REP 0`.
fyrchik commented 2024-03-28 12:09:04 +00:00
Owner
Copy link

My point is that it is really easy to forget EC case and it is can be hard to see what was forgotten.

My point is that it is really easy to forget EC case and it is can be hard to see what was forgotten.
aarifullin commented 2024-03-28 12:42:18 +00:00
Author
Member
Copy link

Alright. To avoid the confusion, I've introduced Replica method and made ReplicaNumberByIndex deprecated. So, Replica allows to check GetCount() and if it's > 0 then the policy is set by REP. EC is set if GetECDataCount() + GetECParityCount() > 0. The SDK's client won't forget this fact since he accesses GetECDataCount(), GetECParityCount()

Alright. To avoid the confusion, I've introduced `Replica` method and made `ReplicaNumberByIndex` deprecated. So, `Replica` allows to check `GetCount()` and if it's `> 0` then the policy is set by `REP`. `EC` is set if `GetECDataCount() + GetECParityCount() > 0`. The SDK's client won't forget this fact since he accesses `GetECDataCount(), GetECParityCount()`
aarifullin force-pushed fix/ec_total_count from b59dac7aee to 1c0244baf1 2024-03-28 09:34:34 +00:00 Compare
aarifullin force-pushed fix/ec_total_count from 1c0244baf1 to a6dffd2cb9 2024-03-28 12:37:17 +00:00 Compare
aarifullin changed title from netmap: Introduce ECShardsCount method to netmap: Introduce Replica method 2024-03-28 12:38:19 +00:00
aarifullin force-pushed fix/ec_total_count from a6dffd2cb9 to e1f70fa79d 2024-03-28 12:59:36 +00:00 Compare
fyrchik approved these changes 2024-03-28 15:29:36 +00:00
object/erasurecode/split.go Outdated
@ -44,12 +44,12 @@ func (c *Constructor) Split(obj *objectSDK.Object, key *ecdsa.PrivateKey) ([]*ob
}
func setIDWithSignature(obj *objectSDK.Object, key *ecdsa.PrivateKey) error {
objectSDK.CalculateAndSetPayloadChecksum(obj)
fyrchik commented 2024-03-28 14:40:16 +00:00
Owner
Copy link

Is there some test that fails with the old behaviour? The change seems pretty important, could you write such test?

Is there some test that fails with the old behaviour? The change seems pretty important, could you write such test?
dstepanov-yadro commented 2024-03-28 15:33:25 +00:00
Member
Copy link

To check: create parts, then validate header by CheckHeaderVerificationFields

To check: create parts, then validate header by `CheckHeaderVerificationFields`
aarifullin commented 2024-03-28 21:26:57 +00:00
Author
Member
Copy link

I added header verification in TestSplitMaxShardCount for both cases. Is that okay?
It seems it is better to verify it not with separte unit-test for setIDWithSignature but when parts are created!

I added header verification in `TestSplitMaxShardCount` for both cases. Is that okay? It seems it is better to verify it not with separte unit-test for `setIDWithSignature` but when parts are created!
dstepanov-yadro reviewed 2024-03-28 15:31:20 +00:00
netmap/policy.go Outdated
@ -184,2 +186,4 @@
}
// Replica returns i-th replica descriptor. Index MUST be in range [0; NumberOfReplicas()).
func (p PlacementPolicy) Replica(i int) *netmap.Replica {
dstepanov-yadro commented 2024-03-28 15:31:15 +00:00
Member
Copy link

netmap.Replica - it is api-go type. I think it is better to add some sdk wrapper.

`netmap.Replica` - it is `api-go` type. I think it is better to add some `sdk` wrapper.
👍 1
aarifullin commented 2024-03-28 15:57:12 +00:00
Author
Member
Copy link

Alright. Actually, the same can be said about object and container - they are also confusing names, because we've got the same in api-go :) I renamed the method to ReplicaDescriptor to make entities more distinguishable

Alright. Actually, the same can be said about `object` and `container` - they are also confusing names, because we've got the same in api-go :) I renamed the method to `ReplicaDescriptor` to make entities more distinguishable
fyrchik commented 2024-03-28 17:30:09 +00:00
Owner
Copy link

The point is thatObject and Container have aliases and are used in node as objectSDK.Object

The point is that`Object` and `Container` have aliases and are used in node as `objectSDK.Object`
aarifullin force-pushed fix/ec_total_count from dc68f99dad to 0dfd57aa6b 2024-03-28 15:55:38 +00:00 Compare
aarifullin requested review from fyrchik 2024-03-28 15:57:22 +00:00
aarifullin force-pushed fix/ec_total_count from 0dfd57aa6b to 34ede4cfaf 2024-03-28 21:25:19 +00:00 Compare
dstepanov-yadro approved these changes 2024-03-29 06:19:08 +00:00
acid-ant approved these changes 2024-03-29 06:48:09 +00:00
aarifullin force-pushed fix/ec_total_count from 34ede4cfaf to 9cf5d4b797 2024-03-29 08:45:53 +00:00 Compare
aarifullin force-pushed fix/ec_total_count from 9cf5d4b797 to 903e33b311 2024-03-29 08:47:17 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-03-29 08:49:09 +00:00
aarifullin requested review from acid-ant 2024-03-29 08:49:09 +00:00
aarifullin force-pushed fix/ec_total_count from 903e33b311 to 0632bfb190 2024-03-29 08:51:46 +00:00 Compare
dstepanov-yadro approved these changes 2024-03-29 09:31:50 +00:00
acid-ant approved these changes 2024-03-29 10:34:19 +00:00
aarifullin force-pushed fix/ec_total_count from 0632bfb190 to ec0cb2169f 2024-03-29 10:48:32 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-03-29 10:48:37 +00:00
aarifullin requested review from acid-ant 2024-03-29 10:48:38 +00:00
dstepanov-yadro approved these changes 2024-03-29 10:57:18 +00:00
fyrchik approved these changes 2024-03-29 10:57:56 +00:00
fyrchik merged commit ec0cb2169f into master 2024-03-29 10:58:13 +00:00
fyrchik referenced this pull request from a commit 2024-03-29 10:58:15 +00:00
[#211] object: Fix setIDWithSignature
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
dstepanov-yadro
fyrchik
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
pool

Tasks related to client pool

  
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
pool
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-sdk-go#211
No description provided.
Delete branch "aarifullin/frostfs-sdk-go:fix/ec_total_count"

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?