Fix missing clause in ar_http_iface_middleware #661
Conversation
Added specifications and improved documentation with examples. see: ArweaveTeam/arweave-dev#710
| @@ -2007,6 +2007,15 @@ handle_get_chunk(OffsetBinary, Req, Encoding) -> | |||
| {{true, Packing}, _StoreID} when RequestedPacking == any -> | |||
| ok = ar_semaphore:acquire(get_chunk, infinity), | |||
| {Packing, ok}; | |||
| {true, _StoreID} -> | |||
There was a problem hiding this comment.
I think this missing clause was addressed in master. I suggest we go with the master version for now (since it's undergone some testing already)
There was a problem hiding this comment.
I was not sure of that. So, if it's the case, I can also close this PR. The documentation below was at first for me to understand the code and thought it could be a good idea to improve it for everybody.
| %% @doc Return the list of all configured storage modules covering the given Offset. | ||
| %%-------------------------------------------------------------------- | ||
| %% @doc Return the list of all configured storage modules covering the | ||
| %% given Offset. This offset is defined by the size of the storage |
There was a problem hiding this comment.
I think in this case the offset is an absolute offset of a byte in the weave. I.e. I'd remove the second sentence "This offset is defined by the size of the storage module (in bytes) and its bucket id (e.g. 51)."
| %% | ||
| %% @see get_all/2 | ||
| %% | ||
| %% == Examples == |
There was a problem hiding this comment.
ditto above, I'm not sure the example is quite right. The Offset parameter is "simply" the absolute offset of a byte in the weave (i.e. a value from 0 to 269830660858102 as of block 1607197)
| %% == Examples == | ||
| %% | ||
| %% ``` | ||
| %% StartOffset = 51*3600000000001. |
There was a problem hiding this comment.
ditto comment above. The offsets aren't generally defined in terms of partition boundaries or storage module boundaries. They are generally offsets of bytes or chunks in the weave.
| %% == Examples == | ||
| %% | ||
| %% ``` | ||
| %% ar_storage_module:has_any(51*3600000000001). |
There was a problem hiding this comment.
ditto above. I'd recommend dropping the 51*XXX syntax as it might be confusing. i.e. one common use of these queries is to, for example, find the storage module containing the recall_byte. The recall_byte is a random value taken from across the full weave that determines the range of chunks to read and hash. When the recall_byte is calculated it si ignorant of partition or storage_module boundaries.
sometimes these methods are also used to respond to a GET /chunk request, in that case a client has provided the offset of a chunk they are interested in, and the server needs to find which storage module to consult to read the chunk. In that case the chunk offset is also not defined in terms of storag modules (e.g. because the client may not know the storage moculde configuration of the server)
This PR also adds specifications and improve documentation to help doing a more deeper investigations in the future.