Github user davisp commented on a diff in the pull request:

    https://github.com/apache/couchdb-couch/pull/185#discussion_r80058806
  
    --- Diff: src/couch_file.erl ---
    @@ -531,27 +537,96 @@ find_header(Fd, Block) ->
         {ok, Bin} ->
             {ok, Bin};
         _Error ->
    -        find_header(Fd, Block -1)
    +        find_header_pread2(Fd, Block -1)
         end.
     
     load_header(Fd, Block) ->
         {ok, <<1, HeaderLen:32/integer, RestBlock/binary>>} =
             file:pread(Fd, Block * ?SIZE_BLOCK, ?SIZE_BLOCK),
    -    TotalBytes = calculate_total_read_len(5, HeaderLen),
    -    case TotalBytes > byte_size(RestBlock) of
    +    TotalSize = total_read_size(5, HeaderLen),
    +    case TotalSize > byte_size(RestBlock) of
         false ->
    -        <<RawBin:TotalBytes/binary, _/binary>> = RestBlock;
    +        <<RawBin:TotalSize/binary, _/binary>> = RestBlock;
         true ->
             {ok, Missing} = file:pread(
                 Fd, (Block * ?SIZE_BLOCK) + 5 + byte_size(RestBlock),
    -            TotalBytes - byte_size(RestBlock)),
    +            TotalSize - byte_size(RestBlock)),
             RawBin = <<RestBlock/binary, Missing/binary>>
         end,
         <<Md5Sig:16/binary, HeaderBin/binary>> =
             iolist_to_binary(remove_block_prefixes(5, RawBin)),
         Md5Sig = couch_crypto:hash(md5, HeaderBin),
         {ok, HeaderBin}.
     
    +
    +%% Read a configurable number of block locations at a time using
    +%% file:pread/2.  At each block location, read ?PREFIX_SIZE (5) bytes;
    +%% if the first byte is <<1>>, assume it's a header block, and the
    +%% next 4 bytes hold the size of the header.
    +-spec find_header_pread2(file:fd(), block_id()) ->
    +    {ok, binary()} | no_valid_header.
    +find_header_pread2(Fd, Block) ->
    +    find_header_pread2(Fd, Block, read_count()).
    +
    +-spec find_header_pread2(file:fd(), block_id(), non_neg_integer()) ->
    +    {ok, binary()} | no_valid_header.
    +find_header_pread2(_Fd, Block, _ReadCount) when Block < 0 ->
    +    no_valid_header;
    +find_header_pread2(Fd, Block, ReadCount) ->
    +    Locations = block_locations(Block, ReadCount),
    +    {ok, DataL} = file:pread(Fd, [{L, ?PREFIX_SIZE} || L <- Locations]),
    +    case locate_header(Fd, header_locations(Locations, DataL)) of
    --- End diff --
    
    There's a lot going on in this line. I'd inline header_locations since its 
also only used once and put a pretty big explanatory comment on how that works. 
Its super subtle that you're relying on the lists:foldl/3 down below to reverse 
your list of locations passed to locate_header/2.
    
    Also, we should rename locate_header/2 to something like 
find_first_valid_header/2. Locate and load seem awfully close that my brain 
seems to lose the semantic difference as I review this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to