refactor historical API #1930
refactor historical API #1930
Conversation
api/indexer/indexer.go
Outdated
| // the following should not happen, as we maintain only blockWindow entries; however, | ||
| // if it does, the following would correct that. |
There was a problem hiding this comment.
Since the block window can change on restart, this comment is not accurate ie. we may hit this when the block window shortens such that we should remove blocks
There was a problem hiding this comment.
comment was deleted along with code refactoring.
api/indexer/indexer.go
Outdated
| // ensure that lastHeight always contains the highest height we've seen. | ||
| i.lastHeight = max(i.lastHeight, blk.Block.Hght) |
There was a problem hiding this comment.
Why do we use this here? We should only ever receive blocks in order.
api/indexer/indexer.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (i *Indexer) Notify(_ context.Context, blk *chain.ExecutedBlock) error { | ||
| i.mu.Lock() |
There was a problem hiding this comment.
Do we really need to grab the lock while writing to the database?
I believe using an atomic should be sufficient and removes the need to hold the lock during database operations.
We only read the last height during GetLatestBlock, if it's updated during that time, it should be fine to return a slightly outdated latest block. If a client performs sequential calls, this should still provide them a monotonically increasing view of the latest block.
There was a problem hiding this comment.
The lock no longer takes place during the database updates.
api/indexer/indexer.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (i *Indexer) Notify(_ context.Context, blk *chain.ExecutedBlock) error { | ||
| i.mu.Lock() | ||
| defer i.mu.Unlock() |
There was a problem hiding this comment.
let's add a blank line after grab and defer releasing the lock for consistent style
api/indexer/indexer.go
Outdated
| i.mu.RLock() | ||
| defer i.mu.RUnlock() | ||
| return i.GetBlockByHeight(i.lastHeight) |
There was a problem hiding this comment.
Same comment here for empty line after deferring the lock
internal/cache/fifo.go
Outdated
| @@ -55,4 +63,5 @@ func (f *FIFO[K, V]) Get(key K) (V, bool) { | |||
| // [WriteLock] is held when this is accessed. | |||
| func (f *FIFO[K, V]) remove(key K) { | |||
| delete(f.m, key) | |||
| f.removedKey = &key | |||
There was a problem hiding this comment.
Hmm it seems fairly odd to pass around this data via an internal pointer modified via the callback in the bounded buffer.
It seems that we added this to return the removed key for the indexer, but I don't think we should modify the fifo cache with this hack to support that specific case. Is there a reason not to use DeleteRange to make sure that we've removed everything below our desired height?
There was a problem hiding this comment.
code refactored.
| type GetBlockByHeightRequest struct { | ||
| Height uint64 `json:"height"` | ||
| BlockID ids.ID `json:"blockID"` | ||
| BlockNumber uint64 `json:"blockNumber"` |
There was a problem hiding this comment.
(out of scope for this PR) Can we add an Encoding argument that can be either json or hex, defaults to hex, and specifies which field in the response to fill out ie. we populate either the block bytes or block field, but not both?
Can we apply the same to the transaction and either populate the JSON of the transaction or the transaction bytes?
There was a problem hiding this comment.
yes, I'll tackle this on the next PR.
There was a problem hiding this comment.
I'd like to have some more unit testing for the server as well.
| @@ -53,8 +56,8 @@ func (c *Client) GetBlockByHeight(ctx context.Context, height uint64, parser cha | |||
| resp := GetBlockClientResponse{} | |||
| err := c.requester.SendRequest( | |||
| ctx, | |||
| "getBlockByHeight", | |||
| &GetBlockByHeightRequest{Height: height}, | |||
| "getBlock", | |||
There was a problem hiding this comment.
Out of scope for this PR - we should move the parser from an argument to each function to an argument to construct the client instance, so that we don't need to pass it around each time
api/indexer/client.go
Outdated
| if found && response.Result != nil { | ||
| success = response.Result.Success | ||
| fee = response.Result.Fee | ||
| } |
There was a problem hiding this comment.
Do we need the check for result to be nil? We should guarantee this is never the case
…rsdk into tsachi/historical-read-api
What ?
This PR is the first refactor ovehaul for the hyper sdk indexer.
The PR addresses consistency and interface functionality.
Changes
GetBlockByHeight,GetBlockByHash,GetLatestBlock) intoGetBlock. The structure parameter forGetBlockwould provide the same functionality previously provided by the individual endpoints.Issue
#1923