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 2020/06/03 16:45:35 UTC
[GitHub] [couchdb] nickva opened a new pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
nickva opened a new pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924
When waiting to accept jobs and scheduling was used, timeout is limited based
on how far time scheduling parameter. When no_schedule option is used, time
scheduling parameter is set to 0 always and so in that case, we have to special
case the limit to return `infinity`.
Later on when we wait for the watch to fire, the actual timeout can still be
limited by separate user specified timeout option, but if user specifies
infinity and sets `#{no_schedule => true}` then we should respect and never
return `{error, not_found}` in response.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434817958
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
From your description:
> When no_schedule option is used, time scheduling parameter is set to 0 always, and so in that case, we have to special-case the limit to return infinity.
I read that as that we should return `infinity` when `Timeout == 0`. This catchall case wouldn't do that because the previous clause catches zero (and all negative integers).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434810325
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
From the description I think this is wrong? Are you missing a `Timeout > 0` in the previous clause's guards?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434824237
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
My description was referencing the logic a few API calls higher and was describing why we need to do this.
In this particular clause `limit_timeout` that wouldn't work since `limit_timeout` is called after we clipped the value to a minimum of 100 msec, so I would have to put in 100 msec or so there so I opted to make it an explicit parameter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434827647
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
Ah. I see that's from higher up. Never mind!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434824237
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
My description was referencing the logic a few API calls higher and was describing why we need to this.
In this particular clause `limit_timeout` that wouldn't work since `limit_timeout` is called after we clipped the value to a minimum of 100 msec, so I would have to put in 100 msec or so there so I opted to make it an explicit parameter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924#discussion_r434816654
##########
File path: src/couch_jobs/src/couch_jobs.erl
##########
@@ -385,10 +385,13 @@ wait_any(Subs, State, Timeout0, ResendQ) when
end.
-limit_timeout(Timeout) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
+limit_timeout(_Timeout, true) ->
+ infinity;
+
+limit_timeout(Timeout, false) when is_integer(Timeout), Timeout < 16#FFFFFFFF ->
Timeout;
-limit_timeout(_Timeout) ->
+limit_timeout(_Timeout, false) ->
Review comment:
In the previous clause we return `infinity` if `NoSched` option is `true`. But that value is not passed directly to `erlfdb:wait(...)` it is then compared with `UserTimeout` https://github.com/apache/couchdb/blob/prototype/fdb-layer/src/couch_jobs/src/couch_jobs.erl#L336 and we pass that to `erlfdb:wait(...)`.
We basically say that when `NoSched` is `true` we shouldn't pay attention the `MaxSTime` parameter when deciding how long or if to timeout at all when we wait to accept jobs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #2924: Fix couch_jobs accept timeout when no_schedule option is used
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #2924:
URL: https://github.com/apache/couchdb/pull/2924
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org