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:
us...@infra.apache.org
With regards,
Apache Git Services
