-
Notifications
You must be signed in to change notification settings - Fork 38
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
upd/Bump minimal go version up to 1.19 #2485
Conversation
5832109
to
4ecf606
Compare
Codecov Report
@@ Coverage Diff @@
## master #2485 +/- ##
==========================================
- Coverage 29.83% 29.83% -0.01%
==========================================
Files 406 406
Lines 31026 31019 -7
==========================================
- Hits 9258 9255 -3
+ Misses 20958 20954 -4
Partials 810 810
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not do this until 0.38.0 is out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also replace https://pkg.go.dev/go.uber.org/atomic with https://pkg.go.dev/sync/[email protected]
Technically, all of nspcc-dev/neo-go#2626, nspcc-dev/neo-go#2627, nspcc-dev/neo-go#2628, nspcc-dev/neo-go#2629 can be relevant here as well. |
lets convert to draft for now @carpawell pls check all places for improvement carefully |
4ecf606
to
6c83d5d
Compare
Signed-off-by: Pavel Karpy <[email protected]>
6c83d5d
to
a087aec
Compare
cmd/neofs-node/config.go
Outdated
@@ -553,8 +557,8 @@ func initCfg(appCfg *config.Config) *cfg { | |||
scriptHash: contractsconfig.Netmap(appCfg), | |||
state: netState, | |||
workerPool: netmapWorkerPool, | |||
needBootstrap: !relayOnly, | |||
reBoostrapTurnedOff: atomic.NewBool(relayOnly), | |||
needBootstrap: !atomicRelayOnly.Load(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous code with relayOnly
looked much better to me than with atomicRelayOnly.Load()
. U could change 2 strings instead of 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hope i got you right (not sure how that could be 2 vs 6) and fixed
cmd/neofs-node/config.go
Outdated
needBootstrap: !relayOnly, | ||
reBoostrapTurnedOff: atomic.NewBool(relayOnly), | ||
needBootstrap: !atomicRelayOnly.Load(), | ||
reBoostrapTurnedOff: atomicRelayOnly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to drop pointer from this field, afaik it doesn't make any sense right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they ask not to copy it, it wont pass go vet
linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.internals.healthStatus.Store(int32(control.HealthStatus_HEALTH_STATUS_UNDEFINED))
c.cfgNetmap.reBoostrapTurnedOff.Store(relayOnly)
below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPE then? can you elaborate on your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you meant to rely on the fact that cfg
struct is used via the pointer always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually embed structures, not pointers if we don't copy the structure anyway (always accessing via pointer). And yes, this allows to skip init in many cases (like below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, changed but explicit create-and-set is better than autoinit-and-set IMO
also changed in one more place where a non-pointer case is suitable
cmd/neofs-node/netmap.go
Outdated
@@ -37,7 +37,7 @@ func newNetworkState() *networkState { | |||
nmStatus.Store(control.NetmapStatus_STATUS_UNDEFINED) | |||
|
|||
return &networkState{ | |||
epoch: atomic.NewUint64(0), | |||
epoch: new(atomic.Uint64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointer is not needed too, we could skip explicit initialization of this field
a087aec
to
a191851
Compare
cmd/neofs-node/config.go
Outdated
needBootstrap: !relayOnly, | ||
reBoostrapTurnedOff: atomic.NewBool(relayOnly), | ||
needBootstrap: !atomicRelayOnly.Load(), | ||
reBoostrapTurnedOff: atomicRelayOnly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually embed structures, not pointers if we don't copy the structure anyway (always accessing via pointer). And yes, this allows to skip init in many cases (like below).
Signed-off-by: Pavel Karpy <[email protected]>
a191851
to
a150fa6
Compare
Signed-off-by: Pavel Karpy <[email protected]>
a150fa6
to
9f76159
Compare
copylocks: resetCaches passes lock by value: github.com/nspcc-dev/neofs-node/cmd/neofs-node.shared contains sync/atomic.Bool contains sync/atomic.noCopy (govet) Introduced by #2485. Signed-off-by: Roman Khimov <[email protected]>
copylocks: resetCaches passes lock by value: github.com/nspcc-dev/neofs-node/cmd/neofs-node.shared contains sync/atomic.Bool contains sync/atomic.noCopy (govet) Introduced by #2485.
No description provided.