You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/09/24 13:13:14 UTC
[GitHub] [couchdb] rnewson opened a new pull request #3764: support maximum changes_duration configuration option
rnewson opened a new pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764
## Overview
Restores the ability for a couchdb administrator to set an upper limit to the duration of a continuous changes feed originally introduced in https://github.com/apache/couchdb/commit/c6ad114ec797874a7eccf741fecb7139b117948b but lost in the FDB work.
## Testing recommendations
Set up a continuous changes feed with a heartbeat, observe that it does not terminate naturally. Set the [fabric] changes_duration parameter to some positive integer value and retest. The changes feed should not return any new updates after that duration and the response will _end_ when the database is next updated (but will not return that update in the response).
## Related Issues or Pull Requests
## Checklist
- [x] Code is written and works correctly
- [ ] Changes are covered by tests
- [x] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715666617
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
Review comment:
I tried to match the original diff as closely as possible but am happy to upgrade to these functions if we don't need to support anything older?
--
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
[GitHub] [couchdb] nickva commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715652735
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
Review comment:
To avoid the `div 1000` could use the more modern (OTP 18+) time functions here:
```
1> erlang:system_time(millisecond).
1632492974842
2> T0 = erlang:system_time(millisecond), timer:sleep(1000), erlang:system_time(millisecond) - T0.
1000
```
--
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
[GitHub] [couchdb] nickva commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715655249
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
+ Max = changes_duration(),
+ case AccumulatedTime > Max of
+ true ->
+ end_sending_changes(Callback, UserAcc4, EndSeq);
+ false ->
+ UserCtx = fabric2_db:get_user_ctx(Db),
+ DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
+ case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
+ {ok, Db2} ->
+ ?MODULE:keep_sending_changes(
+ Args#changes_args{limit = NewLimit},
+ ChangesAcc#changes_acc{
+ db = Db2,
+ user_acc = UserAcc4,
+ seq = EndSeq,
+ prepend = Prepend2,
+ timeout = Timeout,
+ timeout_fun = TimeoutFun
+ },
+ false,
+ T0
+ );
+ _Else ->
+ end_sending_changes(Callback, UserAcc3, EndSeq)
+ end
end;
{stop, UserAcc4} ->
end_sending_changes(Callback, UserAcc4, EndSeq)
end
end
end.
+
+changes_duration() ->
+ %% preserving original (3.x) configuration segment;
+ case config:get("fabric", "changes_duration") of
+ undefined ->
+ infinity;
Review comment:
Could add "infinity" as an explicit config value.
If we don't have it, it impossible to override the value with say `local.ini` when `default.ini` or other higher config layer, already has it set to an explicit value.
--
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
[GitHub] [couchdb] rnewson merged pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson merged pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764
--
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
[GitHub] [couchdb] nickva commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715739686
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
Review comment:
Ah ok, it's not a big deal whichever looks better to you.
We don't support anything lower than OTP 21 on `main` and 20 on `3.x` so all the new time functions should be there already.
--
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715668111
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
+ Max = changes_duration(),
+ case AccumulatedTime > Max of
+ true ->
+ end_sending_changes(Callback, UserAcc4, EndSeq);
+ false ->
+ UserCtx = fabric2_db:get_user_ctx(Db),
+ DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
+ case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
+ {ok, Db2} ->
+ ?MODULE:keep_sending_changes(
+ Args#changes_args{limit = NewLimit},
+ ChangesAcc#changes_acc{
+ db = Db2,
+ user_acc = UserAcc4,
+ seq = EndSeq,
+ prepend = Prepend2,
+ timeout = Timeout,
+ timeout_fun = TimeoutFun
+ },
+ false,
+ T0
+ );
+ _Else ->
+ end_sending_changes(Callback, UserAcc3, EndSeq)
+ end
end;
{stop, UserAcc4} ->
end_sending_changes(Callback, UserAcc4, EndSeq)
end
end
end.
+
+changes_duration() ->
+ %% preserving original (3.x) configuration segment;
+ case config:get("fabric", "changes_duration") of
+ undefined ->
+ infinity;
Review comment:
this is how it works on 3.x. If you want to impose a duration you set a numeric value. If you do not want to impose a duration you delete the entry (config:delete or `changes_duration =`).
--
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
[GitHub] [couchdb] rnewson commented on pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#issuecomment-927665880
Those are two good enhancements and agree they would be nice. My work here simply reintroduces the semantics of this feature as it existed in 3.x (and the default.ini comment makes clear that the ending of the response is contingent on receiving an update).
--
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
[GitHub] [couchdb] nickva commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715650962
##########
File path: rel/overlay/etc/default.ini
##########
@@ -208,6 +208,11 @@ bind_address = 127.0.0.1
;
; Bulk docs transaction batch size in bytes
;update_docs_batch_size = 2500000
+;
+; The upper bound, in milliseconds, that a continuous changes feed will return
+; database results for. The response ends on receipt of the first change to occur
+; after the duration period has lapsed.
+;changes_duration =
Review comment:
It's a bit odd not to have a value. Would "infinity" work here as a default?
--
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
[GitHub] [couchdb] nickva commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715738513
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
+ Max = changes_duration(),
+ case AccumulatedTime > Max of
+ true ->
+ end_sending_changes(Callback, UserAcc4, EndSeq);
+ false ->
+ UserCtx = fabric2_db:get_user_ctx(Db),
+ DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
+ case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
+ {ok, Db2} ->
+ ?MODULE:keep_sending_changes(
+ Args#changes_args{limit = NewLimit},
+ ChangesAcc#changes_acc{
+ db = Db2,
+ user_acc = UserAcc4,
+ seq = EndSeq,
+ prepend = Prepend2,
+ timeout = Timeout,
+ timeout_fun = TimeoutFun
+ },
+ false,
+ T0
+ );
+ _Else ->
+ end_sending_changes(Callback, UserAcc3, EndSeq)
+ end
end;
{stop, UserAcc4} ->
end_sending_changes(Callback, UserAcc4, EndSeq)
end
end
end.
+
+changes_duration() ->
+ %% preserving original (3.x) configuration segment;
+ case config:get("fabric", "changes_duration") of
+ undefined ->
+ infinity;
Review comment:
That makes it impossible to override a numeric entry with a default infinity in the config chain. We used this pattern in a few places and "infinity" as timeout value is standard in erlang as well:
```
src/couch/src/couch_doc.erl: MaxLen = case config:get("couchdb", "max_document_id_length", "infinity") of
src/couch_replicator/src/couch_replicator_httpc.erl: Timeout = case config:get("replicator", "request_timeout", "infinity") of
```
--
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715847663
##########
File path: src/chttpd/src/chttpd_changes.erl
##########
@@ -604,31 +605,49 @@ keep_sending_changes(Args, Acc0, FirstRound) ->
true ->
case wait_updated(Timeout, TimeoutFun, UserAcc3) of
{updated, UserAcc4} ->
- UserCtx = fabric2_db:get_user_ctx(Db),
- DbOptions1 = [{user_ctx, UserCtx} | DbOptions],
- case fabric2_db:open(fabric2_db:name(Db), DbOptions1) of
- {ok, Db2} ->
- ?MODULE:keep_sending_changes(
- Args#changes_args{limit = NewLimit},
- ChangesAcc#changes_acc{
- db = Db2,
- user_acc = UserAcc4,
- seq = EndSeq,
- prepend = Prepend2,
- timeout = Timeout,
- timeout_fun = TimeoutFun
- },
- false
- );
- _Else ->
- end_sending_changes(Callback, UserAcc3, EndSeq)
+ AccumulatedTime = timer:now_diff(os:timestamp(), T0) div 1000,
Review comment:
os:timestamp() is used elsewhere in the same module so I think I'd rather leave this as-is, rather than change them all or have a combination of methods for the same thing.
--
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3764: support maximum changes_duration configuration option
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3764:
URL: https://github.com/apache/couchdb/pull/3764#discussion_r715665954
##########
File path: rel/overlay/etc/default.ini
##########
@@ -208,6 +208,11 @@ bind_address = 127.0.0.1
;
; Bulk docs transaction batch size in bytes
;update_docs_batch_size = 2500000
+;
+; The upper bound, in milliseconds, that a continuous changes feed will return
+; database results for. The response ends on receipt of the first change to occur
+; after the duration period has lapsed.
+;changes_duration =
Review comment:
It would not, because that would be a list rather than `undefined`
--
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