chewbranca commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1412534829
##########
src/fabric/src/fabric_rpc.erl:
##########
@@ -535,6 +535,32 @@ changes_enumerator(#full_doc_info{} = FDI, Acc) ->
changes_enumerator(#doc_info{id = <<"_local/", _/binary>>, high_seq = Seq},
Acc) ->
{ok, Acc#fabric_changes_acc{seq = Seq, pending =
Acc#fabric_changes_acc.pending - 1}};
changes_enumerator(DocInfo, Acc) ->
+ #fabric_changes_acc{
+ db = Db,
+ args = #changes_args{
+ include_docs = IncludeDocs,
+ filter_fun = Filter
+ },
+ pending = Pending
+ } = Acc,
+ #doc_info{high_seq = Seq} = DocInfo,
+ RevsOrDocRevs = couch_changes:filter(Db, DocInfo, Filter, IncludeDocs),
+ % include_docs = false, call `filter/3`, use `couch_changes:open_revs/3`
to read docs.
+ % include_docs = true, call `filter/4`, it will return [revs] or {docs,
revs}.
+ % - {}: use `couch_changes:open_revs/3` to open docs
+ % - []: use `fabric_rpc:doc_member/3` to open docs
+ ChangesRow =
+ case is_tuple(RevsOrDocRevs) of
+ true ->
+ {Docs, Revs} = RevsOrDocRevs,
+ get_changes_row(Revs, Acc, DocInfo, fun get_json_docs/3, Docs);
+ false ->
+ get_changes_row(RevsOrDocRevs, Acc, DocInfo, fun doc_member/3,
{Db, DocInfo})
+ end,
Review Comment:
Hi @jiahuili430, overall I think the approach in the PR looks pretty solid.
I haven't had a chance to do a full pass yet, but one thing that jumps out to
me is the discrepancy of the data types returned by `filter/4` depending on
whether or not we include_docs; where the `filter/3` case returns a list of
`Revs` and `fitler/4` can either return that same `Revs` list or that same
`Revs` list but in `{Docs, Revs}`.
I personally think it's cleaner to have the same general data format
returned from each function arity.
So I suggest:
* `filter/3` returns a list of Revs like normal
* `filter/4` _always_ returns `{Docs, Revs}`, even that's equivalent to
`{[], filter/3(...)}`
Then we can rewrite the above case statement as:
```erlang
%% ...
{Docs, Revs} = couch_changes:filter(Db, DocInfo, Filter, IncludeDocs),
ChangesRow =
case Docs of
[] -> get_changes_row(Revs, Acc, DocInfo, fun get_json_docs/3, Docs);
Docs -> get_changes_row(Revs, Acc, DocInfo, fun doc_member/3, {Db,
DocInfo})
end
```
<hr />
> get_changes_row(Revs, Acc, DocInfo, fun doc_member/3, {Db, DocInfo})
Also, do we need to double pass the `DocInfo` field? Seeing as this is the
only caller of `get_changes_row/5` I think we should be able isolate `Docs vs
{Db, DocInfo}` directly in the `get_changes_row` function or prior to calling
it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
