You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by tonysun83 <gi...@git.apache.org> on 2016/09/05 01:22:24 UTC
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
GitHub user tonysun83 opened a pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56
Make Query Limit Results Configurable
Currently, all_docs are view results are practically unlimited
unless a user passes in a defined limit. This change allows an
administrator/operator to set a limit which takes priority over the
user defined limit. If this property is not set, then the default
value 16#10000000 is used.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-mrview 67462-add-configurable-limit
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-mrview/pull/56.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #56
----
commit 92575f448bbba34466e64ab595aa3ec8af3c96b6
Author: Tony Sun <to...@cloudant.com>
Date: 2016-09-05T01:19:23Z
make query limit results configurable
Currently, all_docs are view results are practically unlimited
unless a user passes in a defined limit. This change allows an
administrator/operator to set a limit which takes priority over the
user defined limit. If this property is not set, then the default
value 16#10000000 is used.
BugzId: 67462
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@tonysun83
> I still think we should make all our index limits configurable, starting with views.
That's a good position while requesting indexes is not cheap. Say, there are some O(N) operations which can cause DoS by requesting a little bit of big data. Otherwise, or if we can stream results chunk by chunk ala changes feed without any harm for server, there is no need in such limitations, even configurable, as they makes things only complicated while it's the client who will cause DoS himself by requesting more than it can process.
I would like to take a look on this feature under such kind of angle. Would the proposed limit save server from some kind of resources drain or it's about protection the clients? Can we make all our indexes steam like and encourage people use them instead? Will that help everyone to solve the problem of requesting unlimited data?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77590126
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
`max_query_limit` confusing. Is it max value I can put into `limit` query parameter?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
Good points, @rnewson. Changing my to -1. I think this need for more wide discussion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77589926
--- Diff: test/couch_mrview_all_docs_tests.erl ---
@@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).
+should_query_with_config_limit_and_skip(Db) ->
--- End diff --
this test is never run.
```/Users/robertnewson/Source/asf/couchdb/src/couch_mrview/test/couch_mrview_all_docs_tests.erl:110: Warning: function should_query_with_config_limit_and_skip/1 is unused```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
and there's no JIRA ticket associated with this change either.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77654169
--- Diff: test/couch_mrview_all_docs_tests.erl ---
@@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).
+should_query_with_config_limit_and_skip(Db) ->
--- End diff --
I will fix the tests. I agree with @kxepal : it should be ```default_query_limit``` across the board. if there are no objections, I will go ahead and use that name
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77595154
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
I just mean it needs to match with the setting in the test, but yes, that's the point, right? An optional forced upper value of ?limit=X.
good catch of section name, it should be `couch_mrview` imo.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77590186
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
But section name `couch_db` is something strange. We don't have any of such.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77550364
--- Diff: include/couch_mrview.hrl ---
@@ -10,6 +10,8 @@
% License for the specific language governing permissions and limitations under
% the License.
+-define(MAX_QUERY_LIMIT, 16#10000000).
--- End diff --
`DEFAULT_MAX_QUERY_LIMIT`, I'm sure a user could specify a larger value, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@tonysun83
With forcing `limit` query parameter we should provide quite clean and simple way to do pagination with the requests. Otherwise people will just send `limit=100500` because they don't care and want to get their data without all these complications. And since pagination is not a simple thing, I fear that latter case will be very popular.
On second hand, limits doesn't helps to protect servers, because it's all depends on documents size which you can include into response. So `limit=1000` may be fine for 1KiB docs, but it's not for `100MiB` while `include_docs=true` is so easy and helpful. Anyway, that's quite strange protection through user API because it doesn't solves any user problem. Why they should care about the server issues?
I think that instead better to try stream views / mango without any limits. You can stop at anytime, remember the offset / seq and start over again. And noone will consume resources more than single view result cost unless someone does buffering.
So these are my thoughts. I'm drifting a bit away from initial idea to set up just some the limit to a new feature that can solve same problem in a bit better fashion. Wouldn't alternative solutions be better in mid/long term? We had unlimited view results all the history and seems like there is no urgency to make rush decision here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@kxepal @rnewson @davisp : now that Couch2.0 is released, I wanted to bring to this back up for discussion. I'm open to suggestions for various configuration names, but I still think we should make all our index limits configurable, starting with views.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@rnewson: thx for pointing that out. I think my new change should adhere to the behavior we want:
1) If there is no config or if it's a non positive integer, then we just use whatever is provided by the user OR the original default. In this case, a user can give a higher limit than 16#10000000 which will be accepted.
2) If a positive config value is there, then it takes precedence over the user defined limit and we use that value instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77600024
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
I see. My point was about thin difference between `default_query_limit` and `max_query_limit`. First one assumes that you can specify any value instead to override the default, even greater than. The second implies (at least for me) that I cannot beat that value in config file, so `?limit=100` with `max_query_limit=10` will still return 10 records back.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77590404
--- Diff: test/couch_mrview_all_docs_tests.erl ---
@@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).
+should_query_with_config_limit_and_skip(Db) ->
--- End diff --
(neither is `/Users/robertnewson/Source/asf/couchdb/src/couch_mrview/test/couch_mrview_map_views_tests.erl:97: Warning: function should_map_with_config_limit/1 is unused`)
This is cause we need to mention them explicitly above, they're not auto-detected
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77600225
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
Hm..I should read the code probably, lol. The `min(Limit, ConfigDefault1)` is quite specific on the intentions. So everything is correct here, you're right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77662244
--- Diff: test/couch_mrview_all_docs_tests.erl ---
@@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).
+should_query_with_config_limit_and_skip(Db) ->
--- End diff --
@tonysun83 Wait. We just figured out that `default_query_limit` doesn't reflects the behaviour.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
-1, the default must be the current behaviour. A partial result from _all_docs is not the same as a full one. What's the use case for forcibly clipping a response?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77550375
--- Diff: include/couch_mrview.hrl ---
@@ -10,6 +10,8 @@
% License for the specific language governing permissions and limitations under
% the License.
+-define(MAX_QUERY_LIMIT, 16#10000000).
--- End diff --
or just `DEFAULT_LIMIT`, this ain't Java. :D
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@rnewson: I'm using the original value here: https://github.com/cloudant/couchdb-couch-mrview/blob/92575f448bbba34466e64ab595aa3ec8af3c96b6/include/couch_mrview.hrl#L76. Isn't that the current default behavior?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77589892
--- Diff: src/couch_mrview_http.erl ---
@@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
- lists:foldl(fun({K, V}, Acc) ->
+ Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
- end, Args1, Props).
-
+ end, Args1, Props),
+ Limit = Args2#mrargs.limit,
+ case config:get_integer("couch_db", " default_query_limit", -1) of
--- End diff --
this should be `max_query_limit`, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
still some more issues but we're getting closer. Note that this cannot merge until 2.0 is cut, we're in feature freeze.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
updated with macros and created corresponding jira ticket
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
If it still planned to be accepted, for the lesser surprise there should be:
1) The way to return good old behaviour back.
2) The warning in the logs that by default we do limit results now, so admins won't lost their time to figure out what's the problem diving into Erlang.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
I still think the default behaviour of _all_docs, _changes and _view should be to return everything that the parameters dictate (that is, we should fix the default, the longstanding large-but-finite default is also a bug).
If we all agree to change that, then it needs to be abundantly clear to the user that they did not get all results. One suggestion is, instead of changing the default, we introduce an optional _maximum_ value for limit and then, if set, reject all requests without an explicit limit parameter. So that's a 400 Bad Request if limit is too high _or_ if it's missing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/56#discussion_r77590448
--- Diff: test/couch_mrview_all_docs_tests.erl ---
@@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).
+should_query_with_config_limit_and_skip(Db) ->
--- End diff --
my suspicion is your new test fails, because of the bug mentioned.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
hrm, very bleh. Ok, please make that a `-define` and update in both places; we'll have to come back to this. A default finite limit for _all_docs seems like a contract-breaking detail to me (even if the actual value is ~268 million). (cc @kocolosk @davisp).
Imo the default behaviour for _all_docs and views is that, unless constrained by startkey/endkey, you could receive any number of rows. Since wee stream them, there's no reason to put an artificial cap here, no matter how generous.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by tonysun83 <gi...@git.apache.org>.
Github user tonysun83 commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
@kxepal: I'm inclined to think that this change helps to protect servers from clients. However, at the same time, we don't want to make it a configurable change that doesn't force a response from the user while giving them different result sets. They might not notice they're a different result set. I like @rnewson's idea of forcing a required limit parameter upon them and fixing the default value to actually become infinity. This way, the old behavior stands but if something changes, the user made aware.
Now we need to consider how this "maximum" field integrates with mango. The original idea for mango was to make limits for both json and text indexes the same across the board (similar to what rnewson's PR for mango). But this change introduces another configurable value. What happens when this "maximum" limit is set to 10, but the mango limit is still the default of 25? Do we simply ignore this "maximum" and use the mango 25 limit or do we enforce the 10? Also, the maximum limit for text indexes is 200 and has bookmarks. So all these factors must be considered. Personally, I'm leaning towards making everything configurable but as it's own separate entity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #56: Make Query Limit Results Configurable
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/56
hrm, this still isn't quite right.
current behaviour, limit defaults to `16#10000000` (268,435,456 in decimal), but a user could specify a higher value and it would work (@tonysun83, right?)
the new behaviour should preserve the same default `16#10000000` and allow a separate and optional upper limit.
What it does right now is always impose an upper limit which defaults to `16#10000000`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---