Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
migrations: finalize migration_5 (share encoding GOB -> SSZ) #2002
migrations: finalize migration_5 (share encoding GOB -> SSZ) #2002
Changes from 6 commits
eb5b7d7
2c6d7e6
8a8d873
ec5df5c
5259c9e
a8258fa
ba4b807
fdeb284
da77d0c
7bdae58
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 20 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L19-L20
Check warning on line 25 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L22-L25
Check warning on line 31 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L27-L31
Check warning on line 40 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L40
Check warning on line 43 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L42-L43
Check warning on line 52 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L48-L52
Check warning on line 58 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L57-L58
Check warning on line 63 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L63
Check warning on line 76 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L73-L76
Check warning on line 79 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L78-L79
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.
as per @moshe-blox recommendation I found
litter
to be super useful for debugging, what do you think of using it like that (for printing detailed error message) ?Alternatively we could use
logger
to print the contents ofshareGOB
andshareSSZ
, but since we also have to returnerror
that means we essentially have to duplicate error message (and it will be logged twice)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.
It seems to have pretty good score, although, would it be enough to just use the logger? I assume the challenge is that zap logger does not log pointer objects with its full properties if to use something like
zap.Any(shareGOB)
?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.
I think
zap
logger should produce full object printout withzap.Any
(although it will probably struggle if struct contains interfaces - #1981),the main reason I don't want to use logger here is that we'll essentially be logging this
GOB share doesn't match corresponding SSZ share, GOB: ...
error twice:and only one of these logged lines will contain full contents of
shareGOB
andshareSSZ
printed out (which is even more confusing - could look like 2 separate/different errors unless you look super carefully)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.
I see.
I’m just slightly hesitant about adding a new dependency and increasing the binary size, especially since it’s only used in one place within the DB migration, which clients would run just once. Although, if @moshe-blox thinks it's OK, then I do not mind
Check warning on line 111 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L84-L111
Check warning on line 115 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L113-L115
Check warning on line 120 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L117-L120
Check warning on line 127 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L126-L127
Check warning on line 148 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L130-L148
Check warning on line 164 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L150-L164
Check warning on line 171 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L166-L171
Check warning on line 183 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L174-L183
Check warning on line 185 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L185
Check warning on line 195 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L187-L195
Check warning on line 197 in migrations/migration_5_share_gob_to_ssz.go
migrations/migration_5_share_gob_to_ssz.go#L197