Cache storage keys #1767
Cache storage keys #1767
Conversation
cla-bot
bot
added
the
s
arbos/retryables/retryable.go
Outdated
| _ = retStorage.ClearByUint64(beneficiaryOffset) | ||
| _ = retStorage.ClearByUint64(timeoutOffset) | ||
| _ = retStorage.ClearByUint64(timeoutWindowsLeftOffset) |
There was a problem hiding this comment.
Why do we ignore these errors ?
There was a problem hiding this comment.
I've added a comment with explanation, but if it would be better I can add the error checks there. The explenation:
we ignore returned error as we expect that if one ClearByUint64 fails, than all consecutive calls to ClearByUint64 will fail with the same error (not modifying state), and then ClearBytes will also fail with the same error (also not modifying state) - and this one we check and return
There was a problem hiding this comment.
please let me know what you think
There was a problem hiding this comment.
Thanks for the explanation! I think it would be a good practice to handle error immediately rather than relying the fact that this will trigger another error below that we handle. It's easier that way to understand for a reader.
You can address it in follow up PR though.
| <-start | ||
| ss := s.OpenCachedSubStorage(subSpaceID) | ||
| if !bytes.Equal(ss.storageKey, expectedKey) { | ||
| errs <- fmt.Errorf("unexpected storage key, want: %v, have: %v", expectedKey, ss.storageKey) |
There was a problem hiding this comment.
nit: got comes before want.
https://google.github.io/styleguide/go/decisions#got-before-want
| select { | ||
| case err := <-errs: | ||
| t.Fatal(err) | ||
| default: | ||
| } |
There was a problem hiding this comment.
For reporting mismatch you should opt for Errorf [0]. One would use Fatalf more fore something that makes rest of the test meaningless, e.g. error when setting up RPC connection or something like that.
And we should display all errors from that channel.
[0] https://google.github.io/styleguide/go/decisions#keep-going
Adds LRU cache for static storage keys.