morph: support reloading morph endpoints with sighup #2998
Conversation
End-rey
requested review from
roman-khimov,
carpawell and
cthulhu-rider
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2998 +/- ##
==========================================
- Coverage 23.15% 23.15% -0.01%
==========================================
Files 789 790 +1
Lines 58815 58830 +15
==========================================
+ Hits 13619 13622 +3
- Misses 44312 44325 +13
+ Partials 884 883 -1 ☔ View full report in Codecov by Sentry. |
pkg/morph/client/reload.go
Outdated
| currentEndpointID := conn.client.Endpoint() | ||
|
|
||
| if slices.Index(c.endpoints, currentEndpointID) == -1 { | ||
| conn.client.Close() |
There was a problem hiding this comment.
Brutal. I'd rather document that we're keeping current connection.
There was a problem hiding this comment.
Added to a Reload function description.
There was a problem hiding this comment.
I mean dropping this code, RPC reconnections are costly for node, so we can avoid them and use current node while it's alive.
There was a problem hiding this comment.
So, I documented in the comment why we're keeping current connection.
There was a problem hiding this comment.
@roman-khimov, this happens only if we are connected to a node that should not be used anymore according to the updated nodes list
There was a problem hiding this comment.
you were connected to node X, and then you updated (successfully) to a list that does not have node X, what is your expectation as an admin?
There was a problem hiding this comment.
ok, i am for switching in that case
There was a problem hiding this comment.
I'm also for switching, it seems logical to me. But if this behavior is documented, then fine.
End-rey
force-pushed
the
1871-support-reloading-morph-endpoints-with-sighup
branch
from
9f93904
to
85609dc
Compare
pkg/morph/client/reload.go
Outdated
|
|
||
| c.cfg.endpoints = cfg.endpoints | ||
|
|
||
| c.endpoints = cfg.endpoints |
There was a problem hiding this comment.
This needs to be protected wrt other threads, btw.
There was a problem hiding this comment.
Here switchLock *sync.RWMutex has been deleted. Do I need to add it again or is there some other way?
There was a problem hiding this comment.
It was created for a different purpose. You can introduce som endpoints RWLock now.
End-rey
force-pushed
the
1871-support-reloading-morph-endpoints-with-sighup
branch
2 times, most recently
from
d205b84
to
f92c1b7
Compare
| @@ -10,8 +10,6 @@ Available for reconfiguration fields: | |||
|
|
|||
| ```yml | |||
| head_timeout: | |||
| cache_size: | |||
There was a problem hiding this comment.
a little bit confused why this happened in this PR. you just noted that it is outdated? a separate commit then?
|
|
||
| c.endpointsLock.Lock() | ||
|
|
||
| c.cfg.endpoints = cfg.endpoints |
There was a problem hiding this comment.
hm, maybe not this PR's zone but why we need two field updates?
There was a problem hiding this comment.
Yeah, I wanted to fix this immediately as well. But would be nice if @End-rey untangles it.
pkg/morph/client/reload.go
Outdated
| currentEndpointID := conn.client.Endpoint() | ||
|
|
||
| if slices.Index(c.endpoints, currentEndpointID) == -1 { | ||
| conn.client.Close() |
There was a problem hiding this comment.
@roman-khimov, this happens only if we are connected to a node that should not be used anymore according to the updated nodes list
Signed-off-by: Andrey Butusov <andrey@nspcc.io>
Signed-off-by: Andrey Butusov <andrey@nspcc.io>
Signed-off-by: Andrey Butusov <andrey@nspcc.io>
End-rey
force-pushed
the
1871-support-reloading-morph-endpoints-with-sighup
branch
2 times, most recently
from
e05904f
to
19aff22
Compare
Add a new function `Client.Reload` that passes the `WithEndpoints` option and updates the `Client` endpoints. Add docs. Closes #1871. Signed-off-by: Andrey Butusov <andrey@nspcc.io>
End-rey
force-pushed
the
1871-support-reloading-morph-endpoints-with-sighup
branch
from
19aff22
to
ac18957
Compare
Closes #1871.