Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netmap: add subnet id to placement policy #182

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Nov 17, 2021

Close #179 .

refs/types.proto Outdated
//
// String representation is base-10 integer value.
//
// JSON representation is a JSON number.
Copy link
Contributor

@cthulhu-rider cthulhu-rider Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suffice it to say that it corresponds to the standard Protocol Buffers JSON format. If you want to explicitly describe the format of this message, then this is an object with a numeric field value.

Btw we do not guarantee the JSON compatibility, so refine the format is optional.

refs/types.proto Outdated
@@ -77,6 +77,18 @@ message OwnerID {
bytes value = 1 [json_name = "value"];
}

// NeoFS subnetwork identifier.
//
// `SubnetID` is a 4-byte long.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suffice it to say that it corresponds to the standard NeoFS binary format which is Protocol Buffers with direct orders of fields (I'm not sure it is actually 4-byte long because of prefixes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc shouldn't really describe the format (we are talking about .proto file after all), but having explicit id size is something I would like to know from the doc. Moved this to the field comment.

// JSON representation is a JSON number.
message SubnetID {
// Integer subnetwork identifier.
fixed32 value = 1 [json_name = "value"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's clearly indicate that zero identifies the zero subnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more about the PlacementPolicy interpretatiion than the id itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have you chosen fixed32?

Copy link
Contributor

@alexvanin alexvanin Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have you chosen fixed32?

fixed32 was mentioned for in-place ID checks in smart contract. With uint32, field will be encoded as varint, which is much harder to decode.

Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
@alexvanin alexvanin merged commit c6691c4 into nspcc-dev:master Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand placement policy with subnetwork field
5 participants