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

Fix/Change morph priority order #1648

Merged
merged 2 commits into from
Aug 4, 2022
Merged

Fix/Change morph priority order #1648

merged 2 commits into from
Aug 4, 2022

Conversation

carpawell
Copy link
Member

@carpawell carpawell commented Aug 2, 2022

To mirror the behaviour of the gateways.

@carpawell carpawell self-assigned this Aug 2, 2022
Sort the endpoint by their priority before the first WS client creation to
start the morph client with the highest priority endpoint.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #1648 (9dc0d7d) into master (4558f30) will increase coverage by 0.02%.
The diff coverage is 26.31%.

@@            Coverage Diff             @@
##           master    #1648      +/-   ##
==========================================
+ Coverage   33.18%   33.21%   +0.02%     
==========================================
  Files         332      332              
  Lines       22706    22717      +11     
==========================================
+ Hits         7536     7545       +9     
- Misses      14552    14554       +2     
  Partials      618      618              
Impacted Files Coverage Δ
pkg/local_object_storage/engine/tree.go 0.00% <0.00%> (ø)
pkg/morph/client/constructor.go 0.00% <0.00%> (ø)
pkg/services/object/acl/acl.go 30.23% <0.00%> (ø)
cmd/neofs-node/config/morph/config.go 100.00% <100.00%> (ø)
pkg/morph/client/multi.go 9.09% <100.00%> (+9.09%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@carpawell carpawell marked this pull request as ready for review August 2, 2022 11:38
KirillovDenis
KirillovDenis previously approved these changes Aug 2, 2022
@carpawell
Copy link
Member Author

Changed: 0/default -> 1 priority.

@@ -41,9 +44,14 @@ func RPCEndpoint(c *config.Config) []client.Endpoint {
break
}

priority := int(config.IntSafe(s, "priority"))
if priority == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use if priority <= 0?

NEOFS_MORPH_RPC_ENDPOINT_1_ADDRESS="wss://rpc2.morph.fs.neo.org:40341/ws"
NEOFS_MORPH_RPC_ENDPOINT_1_PRIORITY=1
NEOFS_MORPH_RPC_ENDPOINT_1_PRIORITY=2
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 add priority to the ENDPOINT_0 explicitly too

Copy link
Member Author

Choose a reason for hiding this comment

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

added zero priority to the 0 endpoint

| Parameter | Type | Default value | Description |
|------------|----------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `address` | `string` | | _WebSocket_ N3 endpoint. |
| `priority` | `int` | `1` | Priority of an endpoint. Endpoint with a higher priority (lower configuration value) has more chance of being used. Endpoints with equal priority are iterated over randomly. |
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand priority - "Endpoint with a higher priority must be / will be used until it's healthy" rather than has more chance

Copy link
Member Author

@carpawell carpawell Aug 4, 2022

Choose a reason for hiding this comment

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

that sentence means that it is possible to have more than one endpoint with the same priority; they are shuffled so it is not guaranteed that the first gonna be used. also, if the most prioritized endpoint failed but then returned, we would still use the less prioritized endpoint after the switch

however, we are planning to have some mechanism that would try to return to the most prioritized endpoint but it is not implemented yet. so it is all about the probability

Copy link

Choose a reason for hiding this comment

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

Weighted probability

section := p.name + ".endpoint.client"
for i := 0; ; i++ {
addr := p.cfg.GetString(fmt.Sprintf("%s.%d.%s", section, i, "address"))
if addr == "" {
break
}

priority := p.cfg.GetInt(section + ".priority")
if priority == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if priority == 0 {
if priority <= 0 {

The lowest value means the highest priority.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell added this to the v0.31.0 milestone Aug 4, 2022
@carpawell carpawell merged commit a97dee0 into nspcc-dev:master Aug 4, 2022
carpawell added a commit that referenced this pull request Aug 4, 2022
Sort the endpoint by their priority before the first WS client creation to
start the morph client with the highest priority endpoint.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell deleted the fix/change-proirity-order branch August 4, 2022 13:11
@alexchetaev alexchetaev added the U3 Regular label Aug 23, 2022
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
Sort the endpoint by their priority before the first WS client creation to
start the morph client with the highest priority endpoint.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
The lowest value means the highest priority.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
U3 Regular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants