You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "jiahuili430 (via GitHub)" <gi...@apache.org> on 2023/11/20 16:33:26 UTC
[PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
jiahuili430 opened a new pull request, #4862:
URL: https://github.com/apache/couchdb/pull/4862
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
When filered `_changes` is triggered, `open_rev()` will increment stats. If query has `include_docs=true`, then `doc_member()` will also increase stats.
To avoid this, add a boolean variable in `open_docs/4` to control whether to increase statistics.
## Testing recommendations
```bash
make eunit apps=chttpd suites=chttpd_changes_test
make eunit apps=couch suites=couch_changes_tests
```
<!-- Describe how we can test your changes.
Does it provide any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
<!-- If your changes affect multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
- [ ] Documentation changes were backported (separated PR) to affected branches
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1443335941
##########
src/couch/src/couch_changes.erl:
##########
@@ -368,6 +395,17 @@ open_revs(Db, DocInfo, Style) ->
OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
[Doc || {ok, Doc} <- OpenResults].
+open_revs2(Db, DocInfo) ->
+ DocInfos = [DocInfo#doc_info{revs = [R]} || R <- DocInfo#doc_info.revs],
+ OpenOpts = [deleted, conflicts],
+ OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
+ Docs = [Doc || {ok, Doc} <- OpenResults],
+ [Doc1 | RestDocs] = Docs,
+ RestRevs = [Doc#doc.revs || Doc <- RestDocs],
+ Conflicts = [{conflicts, [{Pos, RevId} || {Pos, [RevId]} <- RestRevs]}],
Review Comment:
That works! Nicely done.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422774834
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, IncludeDocs) when IncludeDocs ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
Review Comment:
ah you're right, it could be both, we want to pass through to the filter/3 call
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1399530868
##########
src/couch/src/couch_db.erl:
##########
@@ -297,7 +303,13 @@ open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
open_doc(Db, Id, Options) ->
- increment_stat(Db, [couchdb, database_reads]),
+ open_doc(Db, Id, Options, true).
+
+open_doc(Db, Id, Options, ChangeStat) ->
+ case ChangeStat of
+ true -> ?MODULE:increment_stat(Db, [couchdb, database_reads]);
+ false -> ok
+ end,
Review Comment:
I'd put this setting inside the Options list, however shouldn't database_reads be incremented whenever we read? open_doc_int/3 will open the doc, so why wouldn't we count it? It seems the path in question opens the same doc twice? If so, that's not a double counting error in my view, incrementing twice seems correct?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#issuecomment-1819515085
```When filered _changes is triggered, open_rev() will increment stats. If query has include_docs=true, then doc_member() will also increase stats.```
In that code path it sounds like the document _is_ being read twice, so incrementing database_reads twice sounds correct. Of course if we could avoid the second read that would be great.
Did I misunderstand the problem?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
chewbranca commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1399558049
##########
src/couch/src/couch_db.erl:
##########
@@ -297,7 +303,13 @@ open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
open_doc(Db, Id, Options) ->
- increment_stat(Db, [couchdb, database_reads]),
+ open_doc(Db, Id, Options, true).
+
+open_doc(Db, Id, Options, ChangeStat) ->
+ case ChangeStat of
+ true -> ?MODULE:increment_stat(Db, [couchdb, database_reads]);
+ false -> ok
+ end,
Review Comment:
Yeah we should be triggering database reads anytime a read is performed. The bug here is that we read the doc twice, one for include_docs=true and once for the filter, when we should only be doing that once.
It's a bit more complicated in that the js filter will potentially load all the available revs, whereas include_docs does not, so some care will need to be taken there, but we definitely do not want to hide data from stats collection.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422786142
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_should_only_open_doc_once({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector",
+ ParamsIncludeDocs = Params ++ "&include_docs=true",
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, Params),
+ Called = meck:num_calls(couch_db, open_doc, 3),
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, ParamsIncludeDocs),
+ CalledIncludeDocs = meck:num_calls(couch_db, open_doc, 3),
+ ?assertEqual(Called, CalledIncludeDocs).
+
+t_style_all_docs_open_doc_should_different({_, DbUrl}) ->
Review Comment:
tiny nit: `should_different -> should_be_different`
would it work to assert exactly how many times more we expect it to be opened for include docs?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1442283064
##########
src/couch/src/couch_changes.erl:
##########
@@ -368,6 +395,17 @@ open_revs(Db, DocInfo, Style) ->
OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
[Doc || {ok, Doc} <- OpenResults].
+open_revs2(Db, DocInfo) ->
+ DocInfos = [DocInfo#doc_info{revs = [R]} || R <- DocInfo#doc_info.revs],
+ OpenOpts = [deleted, conflicts],
+ OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
+ Docs = [Doc || {ok, Doc} <- OpenResults],
+ [Doc1 | RestDocs] = Docs,
+ RestRevs = [Doc#doc.revs || Doc <- RestDocs],
+ Conflicts = [{conflicts, [{Pos, RevId} || {Pos, [RevId]} <- RestRevs]}],
Review Comment:
Wonder if this will add in deleted conflicts as well. Not sure if we have a test case to check for live vs deleted conflicts but I think we should ensure we follow the exact previous behavior if we emitted deleted conflicts, I think this would work but if we didn't previously we'd be breaking the API
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1421252982
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, IncludeDocs) when IncludeDocs ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
Review Comment:
If this is a clause for when `IncludeDocs` we should match on `false`.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1423393371
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_should_only_open_doc_once({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector",
+ ParamsIncludeDocs = Params ++ "&include_docs=true",
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, Params),
+ Called = meck:num_calls(couch_db, open_doc, 3),
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, ParamsIncludeDocs),
+ CalledIncludeDocs = meck:num_calls(couch_db, open_doc, 3),
+ ?assertEqual(Called, CalledIncludeDocs).
+
+t_style_all_docs_open_doc_should_different({_, DbUrl}) ->
Review Comment:
Updated, and thanks for the review!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422855633
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
For completeness, let's add two more tests which also sets `conflicts=true` parameter, one with `style=all_docs` and one with default style. There we would be checking that the document body returned has `_conflicts: ...` fields as expected.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1442049684
##########
src/couch/src/couch_changes.erl:
##########
@@ -368,6 +395,17 @@ open_revs(Db, DocInfo, Style) ->
OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
[Doc || {ok, Doc} <- OpenResults].
+open_revs2(Db, DocInfo) ->
Review Comment:
Wonder if we should rename this to be a bit more descriptive: `open_all_revs_include_doc`?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#issuecomment-1819524851
> In that code path it sounds like the document is being read twice, so incrementing database_reads twice sounds correct. Of course if we could avoid the second read that would be great.
I'll try to avoid the second read, thanks for the review.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
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 doc_member/3, {Db, DocInfo});
Docs -> get_changes_row(Revs, Acc, DocInfo, fun get_json_docs/3, Docs);
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422855633
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
For completeness, let's add two more test which also sets `conflicts=true` parameter, one with `style=all_docs` and one with default style. There we would be checking that the document body returned has `_conflicts: ...` fields as expected.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1442050851
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -1016,3 +1068,8 @@ seq(<<_/binary>> = Seq) ->
binary_to_integer(NumStr);
seq(null) ->
null.
+
+called_times(ReqFun) ->
+ meck:reset(couch_db),
+ ReqFun(),
+ meck:num_calls(couch_db, open_doc, 3).
Review Comment:
If we wanted to be a bit more precise about the number of calls to open_doc just for our db and only with doc_infos:
```erlang
called_times(ReqFun, DbUrl) ->
meck:reset(couch_db),
ReqFun(),
open_doc_calls(DbUrl).
open_doc_calls(DbUrl) ->
#{path := "/" ++ DbName0} = uri_string:parse(DbUrl),
DbName = list_to_binary(DbName0),
FoldFun = fun([Db, IdOrDocInfo, _Opts], Acc) ->
case {mem3:dbname(couch_db:name(Db)), IdOrDocInfo} of
{DbName, #doc_info{}} -> Acc + 1;
{_, _}-> Acc
end
end,
lists:foldl(FoldFun, 0, meck_history(couch_db, open_doc, 3)).
meck_history(Mod, Fun, Arity) ->
[A || {_, {_, F, A}, _} <- meck:history(Mod), F =:= Fun, length(A) =:= Arity].
```
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1421252932
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
Review Comment:
We could just use `true` for `IncludeDocs` in the function args and match directly on 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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1428257900
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
How about this time? Added special case for `filter/5` when `style=all_docs&conflicts=true&include_docs=true` and `open_revs2()` to modify the response to include the `_conflicts` field.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422460761
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
Review Comment:
good idea!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1414514722
##########
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:
Thank you for the review!
I have updated PR as follows:
- `filter/4` will always return `{Docs, Revs}`;
- `ChangesRow` is based on the case statement:
```erlang
case {Changes, Docs, IncludeDocs} of
{[], _, _} -> {no_pass, ...};
{_, _, false} -> ...;
{_, [], true} -> ...; % open docs in `doc_member/4`
{_, [Doc], true} -> ... % docs were opened in `couch_changes:open_revs/3`, call `get_json_doc/3` to convert docs to json object
end
```
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1421252982
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, IncludeDocs) when IncludeDocs ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
Review Comment:
If this is a clause for when `IncludeDocs` if `false` we should match on `false`.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1423401697
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
Oh interesting. Is that because the document didn't have the `_conflicts` field otherwise.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1423393176
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, true) ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, true) ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, true) ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
+ {[], filter(Db, DocInfo, Filter)}.
+
Review Comment:
Good idea!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1428257900
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
How about this time?
- Added special case for `filter/5` when `style=all_docs&conflicts=true&include_docs=true`,
- and `open_revs2()` to modify the response to include the `_conflicts` field.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#issuecomment-1847251604
Thanks for the script, @nickva. I'll investigate more.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422849935
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, true) ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, true) ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, true) ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
+ {[], filter(Db, DocInfo, Filter)}.
+
Review Comment:
For the 3 filter types we are essentially duplicating the code for filter/4 and filter/3. The only difference seems to be that when we pass `IncludeDocs=true` we also return Docs otherwise we return `[]`. Could we combine the cases so all filter calls are filter/4?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1423392716
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
Review Comment:
To include the `_conflicts` field, it seems I still need to open the doc one more time [here](https://github.com/apache/couchdb/pull/4862/files#diff-e8d45123acc0305191b7564c24d8a56e76b8d50e6c6ca66da3ae5c5d13c20d6bR388-R389).
If this version is acceptable, I'll add more test cases for `view / custom` etc.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
chewbranca commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1412569187
##########
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:
Teehee got nerd sniped a bit on reworking `get_changes_row`. I think we can isolate the loading of the docs from the generate of the row itself, and simplify things a bit. I haven't tested this or anything wild like that, but how about something like:
```diff
diff --git a/src/fabric/src/fabric_rpc.erl b/src/fabric/src/fabric_rpc.erl
index 42544ac40..1e611e131 100644
--- a/src/fabric/src/fabric_rpc.erl
+++ b/src/fabric/src/fabric_rpc.erl
@@ -539,36 +539,40 @@ changes_enumerator(DocInfo, Acc) ->
db = Db,
args = #changes_args{
include_docs = IncludeDocs,
- filter_fun = Filter
+ conflicts = Conflicts,
+ filter_fun = Filter,
+ doc_options = DocOptions
},
pending = Pending
} = Acc,
+ Opts =
+ if
+ Conflicts -> [conflicts | DocOptions];
+ true -> DocOptions
+ end,
#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
+ {Docs, Revs} = couch_changes:filter(Db, DocInfo, Filter, IncludeDocs),
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})
+ case {Docs, IncludeDocs} of
+ %% TODO: is it possible to hit `{[], [_|_], true} = {Revs, Docs, IncludeDocs}`?
+ %% if so, we'll load the docs when we should `no_pass` below
+ {[], true} ->
+ Docs1 = [doc_member({Db, DocInfo}, Opts, Filter)],
+ get_changes_row(Revs, Acc, DocInfo, Docs1);
+ {[], false} ->
+ get_changes_row(Revs, Acc, DocInfo, []);
+ {Docs, true} ->
+ Docs1 = [get_json_docs(Docs, Opts, Filter)],
+ get_changes_row(Revs, Acc, DocInfo, Docs1);
+ {_Docs, false} ->
+ get_changes_row(Revs, Acc, DocInfo, [])
end,
ok = rexi:stream2(ChangesRow),
{ok, Acc#fabric_changes_acc{seq = Seq, pending = Pending - 1}}.
-get_changes_row(Revs, Acc, DocInfo, Fun, Args) ->
+get_changes_row(Revs, Acc, DocInfo, Docs) ->
#fabric_changes_acc{
db = Db,
- args = #changes_args{
- include_docs = IncludeDocs,
- conflicts = Conflicts,
- filter_fun = Filter,
- doc_options = DocOptions
- },
pending = Pending,
epochs = Epochs
} = Acc,
@@ -580,21 +584,13 @@ get_changes_row(Revs, Acc, DocInfo, Fun, Args) ->
{seq, {Seq, uuid(Db), couch_db:owner_of(Epochs, Seq)}}
]};
Results ->
- Opts =
- if
- Conflicts -> [conflicts | DocOptions];
- true -> DocOptions
- end,
{change, [
{pending, Pending - 1},
{seq, {Seq, uuid(Db), couch_db:owner_of(Epochs, Seq)}},
{id, Id},
{changes, Results},
{deleted, Del}
- | if
- IncludeDocs -> [Fun(Args, Opts, Filter)];
- true -> []
- end
+ | Docs
]}
end.
```
and that's kind of a big delta, so here's what it looks like afterwards:
```erlang
changes_enumerator(#full_doc_info{} = FDI, Acc) ->
changes_enumerator(couch_doc:to_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,
conflicts = Conflicts,
filter_fun = Filter,
doc_options = DocOptions
},
pending = Pending
} = Acc,
Opts =
if
Conflicts -> [conflicts | DocOptions];
true -> DocOptions
end,
#doc_info{high_seq = Seq} = DocInfo,
{Docs, Revs} = couch_changes:filter(Db, DocInfo, Filter, IncludeDocs),
ChangesRow =
case {Docs, IncludeDocs} of
%% TODO: is it possible to hit `{[], [_|_], true} = {Revs, Docs, IncludeDocs}`?
%% if so, we'll load the docs when we should `no_pass` below
{[], true} ->
Docs1 = [doc_member({Db, DocInfo}, Opts, Filter)],
get_changes_row(Revs, Acc, DocInfo, Docs1);
{[], false} ->
get_changes_row(Revs, Acc, DocInfo, []);
{Docs, true} ->
Docs1 = [get_json_docs(Docs, Opts, Filter)],
get_changes_row(Revs, Acc, DocInfo, Docs1);
{_Docs, false} ->
get_changes_row(Revs, Acc, DocInfo, [])
end,
ok = rexi:stream2(ChangesRow),
{ok, Acc#fabric_changes_acc{seq = Seq, pending = Pending - 1}}.
get_changes_row(Revs, Acc, DocInfo, Docs) ->
#fabric_changes_acc{
db = Db,
pending = Pending,
epochs = Epochs
} = Acc,
#doc_info{id = Id, high_seq = Seq, revs = [#rev_info{deleted = Del} | _]} = DocInfo,
case [X || X <- Revs, X /= null] of
[] ->
{no_pass, [
{pending, Pending - 1},
{seq, {Seq, uuid(Db), couch_db:owner_of(Epochs, Seq)}}
]};
Results ->
{change, [
{pending, Pending - 1},
{seq, {Seq, uuid(Db), couch_db:owner_of(Epochs, Seq)}},
{id, Id},
{changes, Results},
{deleted, Del}
| Docs
]}
end.
```
Maybe something like that could work?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1412359142
##########
src/couch/src/couch_db.erl:
##########
@@ -297,7 +303,13 @@ open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
open_doc(Db, Id, Options) ->
- increment_stat(Db, [couchdb, database_reads]),
+ open_doc(Db, Id, Options, true).
+
+open_doc(Db, Id, Options, ChangeStat) ->
+ case ChangeStat of
+ true -> ?MODULE:increment_stat(Db, [couchdb, database_reads]);
+ false -> ok
+ end,
Review Comment:
Updated the PR to only read docs once when filtering `_change` is triggered.
May I get another review? Thanks.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1434400995
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -492,6 +507,107 @@ t_view_filter_no_match({_, DbUrl}) ->
?assertEqual({8, 0, []}, changes(DbUrl, Params)),
{200, #{}} = req(delete, DDocUrl ++ "?rev=" ++ binary_to_list(Rev)).
+t_include_docs({_, DbUrl}) ->
+ Params = "?include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_all_docs_include_docs({_, DbUrl}) ->
+ Params = "?style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res1}} = req(get, DbUrl ++ "/_changes" ++ Params),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{
+ <<"_id">> := ?DOC3, <<"_rev">> := <<"2-b">>, <<"_deleted">> := ?DELETED
+ }
+ },
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DDOC2, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res1
+ ),
+ {200, #{<<"results">> := Res2}} = req(post, DbUrl ++ "/_changes" ++ Params),
+ ?assertEqual(Res1, Res2).
+
+t_selector_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_all_docs_include_docs({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector&style=all_docs&include_docs=true",
+ {200, #{<<"results">> := Res}} = req(post, DbUrl ++ "/_changes" ++ Params, Body),
+ ?assertMatch(
+ [
+ #{
+ <<"changes">> := [#{<<"rev">> := <<"2-c">>}, #{<<"rev">> := <<"2-b">>}],
+ <<"doc">> := #{<<"_id">> := ?DOC1, <<"_rev">> := <<"2-c">>}
+ }
+ ],
+ Res
+ ).
+
+t_selector_should_only_open_doc_once({_, DbUrl}) ->
+ Body = #{<<"selector">> => #{<<"_id">> => ?DOC1}},
+ Params = "?filter=_selector",
+ ParamsIncludeDocs = Params ++ "&include_docs=true",
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, Params),
+ Called = meck:num_calls(couch_db, open_doc, 3),
+ meck:reset(couch_db),
+ changes_post(DbUrl, Body, ParamsIncludeDocs),
+ CalledIncludeDocs = meck:num_calls(couch_db, open_doc, 3),
+ ?assertEqual(Called, CalledIncludeDocs).
+
+t_style_all_docs_open_doc_should_different({_, DbUrl}) ->
Review Comment:
I tried to assert the exact number of times it would call `open_doc/3`, but both Spidermonkey and Debian versions affect this number. So I changed them back.
```erlang
% Before:
?assert(FilterCalledTimes < FilterIncludeDocsCalledTimes)
% After:
?assert(FilterCalledTimes == FilterIncludeDocsCalledTimes)
```
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1443324913
##########
src/couch/src/couch_changes.erl:
##########
@@ -368,6 +395,17 @@ open_revs(Db, DocInfo, Style) ->
OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
[Doc || {ok, Doc} <- OpenResults].
+open_revs2(Db, DocInfo) ->
+ DocInfos = [DocInfo#doc_info{revs = [R]} || R <- DocInfo#doc_info.revs],
+ OpenOpts = [deleted, conflicts],
+ OpenResults = [couch_db:open_doc(Db, DI, OpenOpts) || DI <- DocInfos],
+ Docs = [Doc || {ok, Doc} <- OpenResults],
+ [Doc1 | RestDocs] = Docs,
+ RestRevs = [Doc#doc.revs || Doc <- RestDocs],
+ Conflicts = [{conflicts, [{Pos, RevId} || {Pos, [RevId]} <- RestRevs]}],
Review Comment:
It seems the `deleted_conflicts` parameter only exists in [`GET /{db}/{docid}`](https://docs.couchdb.org/en/latest/api/document/common.html#get--db-docid) endpoint.
We can't use it in `_changes`.
Added a filter in `RestRevs` to remove deleted conflict revs from the `_conflicts` field.
Thank you for the review!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 merged PR #4862:
URL: https://github.com/apache/couchdb/pull/4862
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#issuecomment-1843526463
To reduce the size of the PR, wonder if it would make sense to extract the enhanced tests into a separate PR. We don't intend to change the behavior of `_changes` endpoint to tests could be updated first then optimize the doc loading separately.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#issuecomment-1843548291
> To reduce the size of the PR, wonder if it would make sense to extract the enhanced tests into a separate PR. We don't intend to change the behavior of `_changes` endpoint to tests could be updated first then optimize the doc loading separately.
Sure, dropped the first commit.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1422464971
##########
src/couch/src/couch_changes.erl:
##########
@@ -264,6 +264,26 @@ filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}) ->
{ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
filter_revs(Passes, Docs).
+filter(Db, DocInfo, {selector, Style, {Selector, _Fields}}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ Passes = [mango_selector:match(Selector, couch_doc:to_json_obj(Doc, [])) || Doc <- Docs],
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {view, Style, DDoc, VName}, IncludeDocs) when IncludeDocs ->
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_view(Db, DDoc, VName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, {custom, Style, Req0, DDoc, FName}, IncludeDocs) when IncludeDocs ->
+ Req =
+ case Req0 of
+ {json_req, _} -> Req0;
+ #httpd{} -> {json_req, chttpd_external:json_req_obj(Req0, Db)}
+ end,
+ Docs = open_revs(Db, DocInfo, Style),
+ {ok, Passes} = couch_query_servers:filter_docs(Req, Db, DDoc, FName, Docs),
+ {Docs, filter_revs(Passes, Docs)};
+filter(Db, DocInfo, Filter, _IncludeDocs) ->
Review Comment:
It could be true or false, let's leave it as is.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid read docs twice when filtered `_changes` is triggered [couchdb]
Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1442318159
##########
src/chttpd/test/eunit/chttpd_changes_test.erl:
##########
@@ -1016,3 +1068,8 @@ seq(<<_/binary>> = Seq) ->
binary_to_integer(NumStr);
seq(null) ->
null.
+
+called_times(ReqFun) ->
+ meck:reset(couch_db),
+ ReqFun(),
+ meck:num_calls(couch_db, open_doc, 3).
Review Comment:
Updated, thanks for the suggestion.
After adding this filter, I can assert the exact number of calls to `open_doc/3`.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid increment stats twice when filtered `_changes` is triggered [couchdb]
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
chewbranca commented on code in PR #4862:
URL: https://github.com/apache/couchdb/pull/4862#discussion_r1399558049
##########
src/couch/src/couch_db.erl:
##########
@@ -297,7 +303,13 @@ open_doc(Db, IdOrDocInfo) ->
open_doc(Db, IdOrDocInfo, []).
open_doc(Db, Id, Options) ->
- increment_stat(Db, [couchdb, database_reads]),
+ open_doc(Db, Id, Options, true).
+
+open_doc(Db, Id, Options, ChangeStat) ->
+ case ChangeStat of
+ true -> ?MODULE:increment_stat(Db, [couchdb, database_reads]);
+ false -> ok
+ end,
Review Comment:
Yeah we should be incrementing database_reads anytime a read is performed. The bug here is that we read the doc twice, one for include_docs=true and once for the filter, when we should only be doing that once.
It's a bit more complicated in that the js filter will potentially load all the available revs, whereas include_docs does not, so some care will need to be taken there, but we definitely do not want to hide data from stats collection.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org