TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 4 Projects Releases 1 Activity Actions

ir: Set extra wallets on SIGHUP #339

Merged
fyrchik merged 1 commit 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 +61 -36
acid-ant commented 2023-05-12 07:45:28 +00:00
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Copy link

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
Copy link

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
Author
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Author
Member
Copy link

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
Author
Member
Copy link

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 labels
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
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-node#339
No description provided.
Delete branch "acid-ant/frostfs-node:feature/125-ir-sighup-log"

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?