You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@couchdb.apache.org by nickva <gi...@git.apache.org> on 2016/06/23 16:47:48 UTC

[GitHub] couchdb pull request #425: Improve reduce function overlow handling

GitHub user nickva opened a pull request:

    https://github.com/apache/couchdb/pull/425

    Improve reduce function overlow handling

    Add `log` as another option for `query_config.reduce_limit` instead of `true`
    or `false` only. This warns user without crashing the view.
    
    Account for reduce functions code size when checking for overlflow. Code size
    was previously implicitly included in input size, skewing the result of
    overflow check.
    
    Increase threshold for overflow check. Before it was only 200. This
    should prevent false positives in case users have a large accumulator object to
    collect various stats.
    
    (PR-ing to discuss. Ok to postpone this after 2.0 release if not needed a 2.0 bug fix)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/cloudant/couchdb 3046-improve-reduce-overflow

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/couchdb/pull/425.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 #425
    
----
commit 1f6f0de3169301c3471784b9d9826075445a0b5f
Author: Nick Vatamaniuc <va...@gmail.com>
Date:   2016-06-23T16:37:47Z

    Improve reduce function overlow handling
    
    Add `log` as another option for `query_config.reduce_limit` instead of `true`
    or `false` only. This warns user without crashing the view.
    
    Account for reduce functions code size when checking for overlflow. Code size
    was previously implicitly included in input size, skewing the result of
    overflow check.
    
    Increase threshold for overflow check. Before it was only 200. This
    should prevent false positives in case users have a large accumulator object to
    collect various stats.

----


---
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 pull request #425: Improve reduce function overlow handling

Posted by nickva <gi...@git.apache.org>.

Github user nickva commented on a diff in the pull request:

    https://github.com/apache/couchdb/pull/425#discussion_r69325628
  
    --- Diff: share/server/views.js ---
    @@ -32,13 +35,21 @@ var Views = (function() {
         };
         var reduce_line = JSON.stringify(reductions);
         var reduce_length = reduce_line.length;
    +    var input_length =  State.line_length - code_size
         // TODO make reduce_limit config into a number
         if (State.query_config && State.query_config.reduce_limit &&
    -          reduce_length > 200 && ((reduce_length * 2) > State.line_length)) {
    -      var reduce_preview = "Current output: '"+(reduce_line.substring(0,100) + "'... (first 100 of "+reduce_length+" bytes)");
    -      throw(["error", 
    -        "reduce_overflow_error", 
    -        "Reduce output must shrink more rapidly: "+reduce_preview]);
    +          reduce_length > 4096 && ((reduce_length * 2) > input_length)) {
    +      var log_message = [
    +          "Reduce output must shrink more repidly:",
    --- End diff --
    
    Good find. Thanks, Tony!


---
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 issue #425: Improve reduce function overlow handling

Posted by nickva <gi...@git.apache.org>.

Github user nickva commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    @kxepal  It would be useful to have a logging / warning only value for this setting. Is it better to call the value "warning?" or have a new additional one "if enabled don't actually enable, but just log" setting. That might be more confusing... What did you have in mind, what's a better setting?
    
    Sorry, didn't follow this part:
    
    > "But here you also add input_line as is which is much longer than reduce_line
    
    What do you mean by adding the input line? The code does the opposite, it subtracts the code size from the line length so it considers data only. Before it was broken the way it worked. So someone could have created a 2MB  function body and then they could create reduce values as big as 1MB.
    
    In general this is a fix to get this code to be useful. Currently because of how restricted the size is, it is actually disabled in production. Periodically customers without realizing create large reduce values, for example by doing `return values`, so trying to fix it.
    
    > With reduce_limit = "log" you can also eventually cause high IO activity since all these data will get dumped on log 
    
    Log is supposed to be enabled for a while as a diagnostic setting. After a while it would be obvious if it works or not. Notice, the log output only includes the line lengths, specifically to not flood it with too much 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 issue #425: Improve reduce function overlow handling

Posted by kxepal <gi...@git.apache.org>.

Github user kxepal commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    @nickva 
    > have a new additional one "if enabled don't actually enable, but just log" setting. That might be more confusing... What did you have in mind, what's a better setting?
    
    `reduce_limit_debug = true|false` sounds good.
    
    For the rest: sorry, I shouldn't read the code at early morning. I didn't recognized that you return out lines length, not the lines itself. So all the further words are the waste. No strong -1, but config options still have to be different.


---
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 issue #425: Improve reduce function overlow handling

Posted by nickva <gi...@git.apache.org>.

Github user nickva commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    Thanks for taking a look @wohali  and @kxepal 
    
    There is a JIRA issue already: https://issues.apache.org/jira/browse/COUCHDB-3046.
    
    Also rebased on master.


---
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 issue #425: Improve reduce function overlow handling

Posted by nickva <gi...@git.apache.org>.

Github user nickva commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    @kxepal 
    
    The problem is the setting is really a tri-state switch of "disabled", "warning", "enabled". SELinux setup on RHEL works the same (disabled, permissive, where it logs but don't block access, and enabled where it blocks access)
    
    Something like `reduce_limit_debug = true | false` needs an explanation how it interacts with `reduce_limit`. For example reduce `reduce_limit_debug` would only work with `reduce_limit = false`, which is not intuitive. When `reduce_limit = true` there is not much need to log as it already blows up and user will find out.
    
    @rnewson Any thoughts on this?


---
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 issue #425: Improve reduce function overlow handling

Posted by kxepal <gi...@git.apache.org>.

Github user kxepal commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    -1
    
    Two problems:
    
    1. Boolean `reduce_limit` flag eventually starts taking string value "log". Things should be consistent and behaviour should be predictable. Here is not the case.
    
     2. `reduce_line` is truncated with a reason.  For instance, to not blow the browser when it get the MiBs of data back. But here you also add `input_line` as  is which is much longer than `reduce_line` by the design. So things will be get more terrible.  With `reduce_limit = "log"` you can also eventually cause high IO activity since all these data will get dumped on log file no matter that debug logs are turned off.


---
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 issue #425: Improve reduce function overlow handling

Posted by kxepal <gi...@git.apache.org>.

Github user kxepal commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    @nickva 
    Hmm....ok. I still don't like it, but cannot propose anything better and the 
    
    > For example reduce reduce_limit_debug would only work with reduce_limit = false, which is not intuitive.
    
    Is indeed more confusing, so the problem is not solved, but just moved to the else domain.


---
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 issue #425: Improve reduce function overlow handling

Posted by wohali <gi...@git.apache.org>.

Github user wohali commented on the issue:

    https://github.com/apache/couchdb/pull/425
  
    @nickva Looks like @kxepal changed his -1 to a -0(ish), and the build passes (I'm rerunning the one platform that had a transient failure but expect success). Would you care to merge this change?
    
    I'd like to see a JIRA ticket for this as well, and a test case if it makes sense.


---
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.
---