Miscellaneous code quality improvements #7414
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7414 +/- ##
==========================================
- Coverage 60.16% 59.98% -0.18%
==========================================
Files 424 419 -5
Lines 30669 30252 -417
==========================================
- Hits 18451 18148 -303
+ Misses 9185 9113 -72
+ Partials 3033 2991 -42 |
| @@ -143,6 +144,8 @@ func (s *Service) ReceiveBlockBatch(ctx context.Context, blocks []*ethpb.SignedB | |||
| } | |||
|
|
|||
| if err := s.VerifyWeakSubjectivityRoot(s.ctx); err != nil { | |||
| // log.Fatalf will prevent defer from being called | |||
| cleanup() | |||
There was a problem hiding this comment.
why not just call span.End() here instead of introducing a third variable?
We do this in other places as well (e.g. call span.End())
There was a problem hiding this comment.
There was a problem hiding this comment.
I wanted to follow the same pattern everywhere, but I agree that span.End can just be called directly.
There was a problem hiding this comment.
Issue is this pattern is not used anywhere in the code base so this actually creates inconsistency.
Here's a few options:
1.) Revert this PR to use span.End and we stick with span.End
2.) Revert this PR to use span.End and in the subsequent PR we replace all with cleanup()
3.) In this PR, we replace all span.End usages with cleanup()
I don't have a strong preference the option, you can choose one, but i do want some consistencies
There was a problem hiding this comment.
discussed offline and this was resolved
| @@ -143,6 +144,8 @@ func (s *Service) ReceiveBlockBatch(ctx context.Context, blocks []*ethpb.SignedB | |||
| } | |||
|
|
|||
| if err := s.VerifyWeakSubjectivityRoot(s.ctx); err != nil { | |||
| // log.Fatalf will prevent defer from being called | |||
There was a problem hiding this comment.
I think the real problem is that we shouldn't be using log.Fatalf. This isn't a graceful shutdown and can cause other issues like corrupt data writes: #7382
There was a problem hiding this comment.
I absolutely agree. We can discuss this during the OKRs meeting since I am about to take a look at logging in general.
What type of PR is this?
Other - code quality improvements
What does this PR do? Why is it needed?
This PR fixes miscellaneous code quality issues found by the DeepSource tool here: https://deepsource.io/gh/rkapka/prysm/. And some things that GoLand complained about.
Which issues(s) does this PR fix?
Fixes #6747
Other notes for review
N/A