Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add Global timeout for map/reduce queries #1766

Merged
merged 2 commits into from Jan 21, 2019

Conversation

@garrensmith
Copy link
Member

@garrensmith garrensmith commented Nov 27, 2018

Overview

This makes the global timeout for a map/reduce and all_docs request
configurable via the config. It separates the config into global queries
and partition queries so that it is possible to make the global timeout
is for partitioned queries

Testing recommendations

Add the global_partition_view_timeout to the [fabric] config. You can then make queries and see if they timeout based on the value set.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@davisp
Copy link
Member

@davisp davisp commented Nov 27, 2018

Looks good but obviously as currently written should wait till after we finish partition queries.

Loading

Copy link
Member

@davisp davisp left a comment

Decided there's actually a few necessary tweaks but they're small.

Loading

global_view_timeout(Args) ->
PartitionQuery = couch_mrview_util:get_extra(Args, partition, false),
case PartitionQuery of
false -> timeout("global_view_timeout", "infinity");
Copy link
Member

@davisp davisp Nov 27, 2018

Choose a reason for hiding this comment

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

You're doubling up the _timeout suffix cause the timeout/2 call appends that as well. Also global_view_timeout should probably be shortened to just view_timeout because you're looking at global vs partitioned. Also global_partition is a bit of a misnomer and should just be partition_view_timeout.

Also I think the SOP is to add commented out config options with their default values in default.ini. If the other timeout config options aren't already present I'd add all five settings in a separate commit on this PR.

Loading

Copy link
Member Author

@garrensmith garrensmith Nov 29, 2018

Choose a reason for hiding this comment

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

I've removed the global_ part for the names. The reason I added that is that rexi has two timeouts. A timeout for how long to wait for each response from a node and then an overall global timeout for the node to send all responses. The view and partition_view are for the second situation where as the all_docs and request timeouts are fo the individual requests.

Loading

@davisp davisp closed this Nov 27, 2018
@garrensmith garrensmith changed the base branch from feature/user-partitioned-databases-davisp to feature/database-partitions Nov 29, 2018
@garrensmith garrensmith reopened this Nov 29, 2018
@garrensmith garrensmith force-pushed the add-partition-timeout branch from f367840 to e4b9104 Nov 29, 2018
@davisp
Copy link
Member

@davisp davisp commented Nov 29, 2018

Whoops. No idea how I managed to close this. Definitely wasn't on purpose.

Loading

Copy link
Member

@davisp davisp left a comment

One last minor nit.

Loading

@@ -152,6 +152,13 @@ all_docs_timeout() ->
attachments_timeout() ->
timeout("attachments", "600000").

global_view_timeout(Args) ->
Copy link
Member

@davisp davisp Nov 29, 2018

Choose a reason for hiding this comment

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

Still seems odd to be calling this global_view_timeout when it applies to both global and partitioned.

Loading

Copy link
Member Author

@garrensmith garrensmith Dec 3, 2018

Choose a reason for hiding this comment

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

I've changed the name

Loading

@davisp davisp force-pushed the feature/database-partitions branch 10 times, most recently from 4da3bab to fc696e8 Dec 7, 2018
@davisp davisp force-pushed the feature/database-partitions branch 11 times, most recently from 90be3b0 to 6c2785a Dec 19, 2018
@davisp davisp force-pushed the feature/database-partitions branch from 6c2785a to 005b442 Dec 19, 2018
@davisp davisp force-pushed the feature/database-partitions branch from 005b442 to 60bbe3f Dec 20, 2018
@davisp davisp force-pushed the feature/database-partitions branch 5 times, most recently from be1fe52 to 31a4a53 Jan 8, 2019
@davisp davisp force-pushed the feature/database-partitions branch 7 times, most recently from 04146c3 to 51a482e Jan 16, 2019
@davisp davisp force-pushed the feature/database-partitions branch 2 times, most recently from 8cd68be to 91af772 Jan 18, 2019
@davisp davisp closed this Jan 18, 2019
@garrensmith garrensmith changed the base branch from feature/database-partitions to master Jan 21, 2019
@garrensmith garrensmith reopened this Jan 21, 2019
garrensmith added 2 commits Jan 21, 2019
This makes the global timeout for a map/reduce and all_docs request
configurable via the config. It separates the config into global queries
and partition queries so that it is possible to make the global timeout
is for partitioned queries
@garrensmith garrensmith force-pushed the add-partition-timeout branch from 23674ad to dadc3ff Jan 21, 2019
davisp
davisp approved these changes Jan 21, 2019
@davisp
Copy link
Member

@davisp davisp commented Jan 21, 2019

+1

Loading

@garrensmith garrensmith merged commit 33e3625 into apache:master Jan 21, 2019
1 check passed
Loading
@wohali wohali mentioned this pull request Feb 7, 2019
@garrensmith garrensmith deleted the add-partition-timeout branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants