Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make view query limits configurable #1804

Merged
merged 1 commit into from
Jan 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rel/overlay/etc/default.ini
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ os_process_limit = 100
; Timeout for how long a response from a busy view group server can take.
; "infinity" is also a valid configuration value.
;group_info_timeout = 5000
;query_limit = 268435456
;partition_query_limit = 268435456

[mango]
; Set to true to disable the "index all fields" text index, which can lead
Expand Down
3 changes: 2 additions & 1 deletion src/couch_mrview/include/couch_mrview.hrl
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
view_states=nil
}).

-define(MAX_VIEW_LIMIT, 16#10000000).

-record(mrargs, {
view_type,
Expand All @@ -74,7 +75,7 @@
keys,

direction = fwd,
limit = 16#10000000,
limit = ?MAX_VIEW_LIMIT,
skip = 0,
group_level = 0,
group = undefined,
Expand Down
30 changes: 27 additions & 3 deletions src/couch_mrview/src/couch_mrview_util.erl
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,10 @@ fold_reduce({NthRed, Lang, View}, Fun, Acc, Options) ->
couch_btree:fold_reduce(Bt, WrapperFun, Acc, Options).


validate_args(Db, DDoc, Args) ->
validate_args(Db, DDoc, Args0) ->
{ok, State} = couch_mrview_index:init(Db, DDoc),
validate_args(State, Args).
Args1 = apply_limit(State#mrst.partitioned, Args0),
validate_args(State, Args1).


validate_args(#mrst{} = State, Args0) ->
Expand All @@ -523,6 +524,28 @@ validate_args(#mrst{} = State, Args0) ->
end.


apply_limit(ViewPartitioned, Args) ->
LimitType = case ViewPartitioned of
true -> "partition_query_limit";
false -> "query_limit"
end,

MaxLimit = config:get_integer("query_server_config",
LimitType, ?MAX_VIEW_LIMIT),

% Set the highest limit possible if a user has not
% specified a limit
Args1 = case Args#mrargs.limit == ?MAX_VIEW_LIMIT of
true -> Args#mrargs{limit = MaxLimit};
false -> Args
end,

if Args1#mrargs.limit =< MaxLimit -> Args1; true ->
Fmt = "Limit is too large, must not exceed ~p",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong here but I'm not immediately seeing why. The view_multi_key_design.js test has failed with this error message on each worker. Perhaps we left a bad config from an unrelated test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the issue. It was because of the fix_skip_and_limit function https://github.com/apache/couchdb/blob/master/src/fabric/src/fabric_view.erl#L380

We made the max limit greater than the defined max limit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue there but I'm not seeing what changed to account for it.

mrverror(io_lib:format(Fmt, [MaxLimit]))
end.


validate_all_docs_args(Db, Args0) ->
Args = validate_args(Args0),

Expand All @@ -533,7 +556,8 @@ validate_all_docs_args(Db, Args0) ->
{false, <<_/binary>>} ->
mrverror(<<"`partition` parameter is not supported on this db">>);
{_, <<_/binary>>} ->
apply_all_docs_partition(Args, Partition);
Args1 = apply_limit(true, Args),
apply_all_docs_partition(Args1, Partition);
_ ->
Args
end.
Expand Down
82 changes: 64 additions & 18 deletions test/elixir/test/partition_all_docs_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -116,36 +116,82 @@ defmodule PartitionAllDocsTest do
assert ids == ["foo:22"]
end

test "partition all docs can set query limits", context do
set_config({"query_server_config", "partition_query_limit", "2000"})

db_name = context[:db_name]
create_partition_docs(db_name)
create_partition_ddoc(db_name)

url = "/#{db_name}/_partition/foo/_all_docs"

resp =
Couch.get(url,
query: %{
limit: 20
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 20

resp = Couch.get(url)
assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 50

resp =
Couch.get(url,
query: %{
limit: 2000
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 50

resp =
Couch.get(url,
query: %{
limit: 2001
}
)

assert resp.status_code == 400
%{:body => %{"reason" => reason}} = resp
assert Regex.match?(~r/Limit is too large/, reason)

resp =
Couch.get(url,
query: %{
limit: 2000,
skip: 25
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 25
end

# This test is timing based so it could be a little flaky.
# If that turns out to be the case we should probably just skip it
test "partition _all_docs with timeout", context do
on_exit(fn ->
resp = Couch.get("/_membership")
%{:body => body} = resp

Enum.each(body["all_nodes"], fn node ->
resp = Couch.put("/_node/#{node}/_config/fabric/partition_view_timeout", body: "\"3600000\"")
assert resp.status_code == 200
end)
end)

resp = Couch.get("/_membership")
%{:body => body} = resp

Enum.each(body["all_nodes"], fn node ->
resp = Couch.put("/_node/#{node}/_config/fabric/partition_view_timeout", body: "\"1\"")
assert resp.status_code == 200
end)
set_config({"fabric", "partition_view_timeout", "1"})

db_name = context[:db_name]
create_partition_docs(db_name)

retry_until(fn ->
url = "/#{db_name}/_partition/foo/_all_docs"

case Couch.get(url) do
%{:body => %{"reason" => reason}} ->
Regex.match?(~r/not be processed in a reasonable amount of time./, reason)
_ ->

_ ->
false
end
end)
Expand Down
60 changes: 60 additions & 0 deletions test/elixir/test/partition_view_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,66 @@ defmodule ViewPartitionTest do
]
end

test "partition query can set query limits", context do
set_config({"query_server_config", "partition_query_limit", "2000"})

db_name = context[:db_name]
create_partition_docs(db_name)
create_partition_ddoc(db_name)

url = "/#{db_name}/_partition/foo/_design/mrtest/_view/some"

resp =
Couch.get(url,
query: %{
limit: 20
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 20

resp = Couch.get(url)
assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 50

resp =
Couch.get(url,
query: %{
limit: 2000
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 50

resp =
Couch.get(url,
query: %{
limit: 2001
}
)

assert resp.status_code == 400
%{:body => %{"reason" => reason}} = resp
assert Regex.match?(~r/Limit is too large/, reason)

resp =
Couch.get(url,
query: %{
limit: 2000,
skip: 25
}
)

assert resp.status_code == 200
ids = get_ids(resp)
assert length(ids) == 25
end

test "include_design works correctly", context do
db_name = context[:db_name]

Expand Down