TrueCloudLab
/
frostfs-node
18
1
Fork 20
Code Issues 88 Pull Requests 8 Actions Releases Activity

ir: Set extra wallets on SIGHUP #339

Merged
fyrchik merged 1 commits from acid-ant/frostfs-node:feature/125-ir-sighup-log into master 2023-05-16 12:47:45 +00:00
Conversation 0 Commits 1 Files Changed 7
acid-ant commented 2023-05-12 07:45:28 +00:00
Collaborator

Close #125

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #125 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-05-12 07:45:38 +00:00
acid-ant requested review from storage-core-developers 2023-05-12 07:45:39 +00:00
fyrchik reviewed 2023-05-12 07:53:04 +00:00
internal/logs/logs.go Outdated
@ -415,6 +415,7 @@ const (
FrostFSIRApplicationStopped = "application stopped" // Info in ../node/cmd/frostfs-ir/main.go
FrostFSIRCouldntCreateRPCClientForEndpoint = "could not create RPC client for endpoint" // Debug in ../node/pkg/morph/client/constructor.go
FrostFSIRCreatedRPCClientForEndpoint = "created RPC client for endpoint" // Info in ../node/pkg/morph/client/constructor.go
FrostFSIRReloadExtraWallets = "reload extra wallets" // Info in ../node/cmd/frostfs-ir/config.go
fyrchik commented 2023-05-12 07:46:38 +00:00
Owner

Do we need this message if wallets haven't changed?

Do we need this message if wallets haven't changed?
acid-ant commented 2023-05-15 06:50:23 +00:00
Poster
Collaborator

That was done in the same manner as it done for pprof and metrics. Should we revise this?

That was done in the same manner as it done for pprof and metrics. Should we revise this?
fyrchik marked this conversation as resolved
pkg/innerring/processors/alphabet/process_emit.go Outdated
@ -13,6 +13,8 @@ import (
const emitMethod = "emit"
func (ap *Processor) processEmit() {
ap.mtx.RLock()
fyrchik commented 2023-05-12 07:50:29 +00:00
Owner

We need the mutex only to access parsedWallets field, can we reduce it's scope?
Blocking SIGHUP processing because of some network errors or timeouts is undesireable.

We need the mutex only to access `parsedWallets` field, can we reduce it's scope? Blocking SIGHUP processing because of some network errors or timeouts is undesireable.
fyrchik commented 2023-05-12 07:51:43 +00:00
Owner

I am now wondering why do we need extraLen in transferGasToExtraNodes, we still access ap.parsedWallets, could just provide a slice, so we have a single read at line 57.

I am now wondering why do we need `extraLen` in `transferGasToExtraNodes`, we still access `ap.parsedWallets`, could just provide a slice, so we have a single read at line 57.
acid-ant commented 2023-05-12 09:20:55 +00:00
Poster
Collaborator

I used a mutex because I didn't want to reorganize existing code. I see that it is possible to make "processEmit" a little more understandable and avoid using a mutex. Will do that.

I used a mutex because I didn't want to reorganize existing code. I see that it is possible to make "processEmit" a little more understandable and avoid using a mutex. Will do that.
fyrchik marked this conversation as resolved
pkg/innerring/processors/alphabet/processor.go Outdated
@ -53,6 +54,7 @@ type (
morphClient morphClient
irList Indexer
storageEmission uint64
mtx *sync.RWMutex
fyrchik commented 2023-05-12 07:47:58 +00:00
Owner

Could you mention in comment, what does this mutex protect?

Could you mention in comment, what does this mutex protect?
👍 2
acid-ant commented 2023-05-15 06:50:57 +00:00
Poster
Collaborator

Removed mutex usage, please review.

Removed mutex usage, please review.
fyrchik marked this conversation as resolved
dstepanov-yadro approved these changes 2023-05-12 14:10:05 +00:00
acid-ant force-pushed feature/125-ir-sighup-log from 8452d7935b to 579340f2af 2023-05-15 06:44:39 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from 579340f2af to d43eefbd73 2023-05-15 06:44:48 +00:00 Compare
aarifullin reviewed 2023-05-15 08:32:55 +00:00
pkg/innerring/processors/alphabet/process_emit.go Outdated
@ -96,2 +97,2 @@
if extraLen != 0 {
err := ap.morphClient.BatchTransferGas(ap.parsedWallets, gasPerNode)
func (ap *Processor) transferGasToExtraNodes(pw []util.Uint160, gasPerNode fixedn.Fixed8) {
if len(pw) != 0 {
aarifullin commented 2023-05-15 08:32:55 +00:00
Collaborator

I know that the length cannot be < 0 but let it be len(pw) > 0 :)

I know that the length cannot be `< 0` but let it be `len(pw) > 0` :)
👍 1
aarifullin approved these changes 2023-05-15 08:35:05 +00:00
acid-ant force-pushed feature/125-ir-sighup-log from d43eefbd73 to f62e1f42e4 2023-05-15 09:18:01 +00:00 Compare
aarifullin approved these changes 2023-05-15 11:05:26 +00:00
acid-ant force-pushed feature/125-ir-sighup-log from f62e1f42e4 to c3365270e5 2023-05-15 12:27:10 +00:00 Compare
fyrchik reviewed 2023-05-15 16:11:21 +00:00
pkg/innerring/processors/alphabet/process_emit.go Outdated
@ -53,3 +54,3 @@
nmNodes := networkMap.Nodes()
nmLen := len(nmNodes)
extraLen := len(ap.parsedWallets)
pw := ap.parsedWallets
fyrchik commented 2023-05-15 16:11:21 +00:00
Owner

Why is there no mutex taken? It seems processEmit can be called concurrently.

Why is there no mutex taken? It seems `processEmit` can be called concurrently.
acid-ant commented 2023-05-16 06:09:01 +00:00
Poster
Collaborator

Previously, mutex used to protect the whole calculation and sending gas to extra wallets. We had a decision to avoid doing something related to network under the mutex. What the reason to protect the change of pointer on array?

Previously, mutex used to protect the whole calculation and sending gas to extra wallets. We had a decision to avoid doing something related to network under the mutex. What the reason to protect the change of pointer on array?
acid-ant commented 2023-05-16 06:51:06 +00:00
Poster
Collaborator

Now I know more about slices in GO :) I'll add mutex.

Now I know more about slices in GO :) I'll add mutex.
acid-ant commented 2023-05-16 07:09:25 +00:00
Poster
Collaborator

Mutex added, please review.

Mutex added, please review.
acid-ant force-pushed feature/125-ir-sighup-log from c3365270e5 to f114708d0f 2023-05-16 07:08:45 +00:00 Compare
acid-ant force-pushed feature/125-ir-sighup-log from f114708d0f to 274c4be222 2023-05-16 10:39:12 +00:00 Compare
fyrchik approved these changes 2023-05-16 12:47:37 +00:00
fyrchik merged commit 6f47c75e43 into master 2023-05-16 12:47:45 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dstepanov-yadro
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest   
P1

High   
P2

Medium   
P3

Low   
badger

Badger DB related issues   
frostfs-adm
   
frostfs-cli
   
frostfs-ir
   
frostfs-lens
   
frostfs-node
   
good first issue

Good for newcomers   
triage
  
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 Label
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
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
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-node#339
There is no content yet.
Delete Branch "acid-ant/frostfs-node:feature/125-ir-sighup-log"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?