[GitHub] chewbranca commented on a change in pull request #1605: Feature/user partitioned databases

[GitBox <gi...@apache.org>]

chewbranca commented on a change in pull request #1605: Feature/user partitioned databases
URL: https://github.com/apache/couchdb/pull/1605#discussion_r227596683
 
 

 ##########
 File path: src/mem3/src/mem3_util.erl
 ##########
 @@ -34,6 +35,27 @@ hash(Item) when is_binary(Item) ->
 hash(Item) ->
     erlang:crc32(term_to_binary(Item)).
 
+docid_hash(DocId) when is_binary(DocId) ->
+    docid_hash(DocId, []).
+
+docid_hash(<<"_design/", _/binary>> = DocId, _Options) ->
+    erlang:crc32(DocId); % design docs are never placed by partition
+
+docid_hash(DocId, []) when is_binary(DocId) ->
+    docid_hash(DocId, [{partitioned, false}]);
+
+docid_hash(DocId, [{partitioned, false}]) when is_binary(DocId) ->
 
 Review comment:
   I know up the call stack this parameter is referred to as `HashOptions` but this current structure will prevent this from ever actually being an options list given that it's matching on a single element list. So adding another option to `HashOptions` will cause a function_clause error here, so why have it at all? This seems like unnecessary future proofing, especially given that to use this field as an actual options list will require restructuring the function definitions as you can't define a function head to match on _an_ element in a proplist.
   
   I suggest either making this an actual proplist and using a variation of `couch_util:get_value(partioned, Options)`, or eliminating the list entirely and making this a boolean parameter. You could rewrite this series of functions as something like:
   
   ```erlang
   docid_hash(DocId) ->
     docid_hash(DocId, false).
   
   docid_hash(DocId, false) ->
     erlang:crc32(DocId);
   docid_hash(<<"_design/", _/binary>>=DocId, _) ->
     erlang:crc32(DocId);
   docid_hash(DocId, true) ->
       case binary:split(DocId, <<":">>) of
           [Partition, _Rest] ->
               erlang:crc32(Partition);
           _ ->
               throw({illegal_docid, <<"doc id must be of form partition:id">>})
       end.
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services