You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2021/10/01 16:22:46 UTC
[GitHub] [mesos] xiao66xiang opened a new pull request #410: Fix old sandboxes not being GC'ed issue
xiao66xiang opened a new pull request #410:
URL: https://github.com/apache/mesos/pull/410
Some old sandboxes were not GC'ed because Duration `d` became negative and caused `Timeout::in(d)` to overflow. Hence we got a big number for `removalTime` and those old sandboxes were never sent to remove.
Jira: https://issues.apache.org/jira/browse/MESOS-10232
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali merged pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
cf-natali merged pull request #410:
URL: https://github.com/apache/mesos/pull/410
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] mengzhugithub commented on pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
mengzhugithub commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-942930828
LGTM. @bmahler let's merge this?
@xiao66xiang please add the mesos test in a separate PR. thanks!
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] xiao66xiang commented on pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
xiao66xiang commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-933890736
Yes I'll send a separate PR for the mesos tests.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] bmahler commented on pull request #410: Fix old sandboxes not being GC'ed issue
Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-932460110
While you're here, would you mind updating it to grab a single clock now value at the beginning? The double call makes it tricky to reason about the overflow logic holding true on the second calls as well.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] xiao66xiang commented on a change in pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
xiao66xiang commented on a change in pull request #410:
URL: https://github.com/apache/mesos/pull/410#discussion_r730161866
##########
File path: 3rdparty/libprocess/include/process/timeout.hpp
##########
@@ -34,10 +34,20 @@ class Timeout
// from now.
static Timeout in(const Duration& duration)
{
+ Time now = Clock::now();
+ if (duration < Duration::zero()) {
Review comment:
In `gc.cpp`, it will order the files based on Timeout, so the oldest file gets deleted first. The constructor of the `Timeout` class can also take time in the past. But it's also fine if we just return `now`.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-944894888
@xiao66xiang Thanks!
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] bmahler commented on pull request #410: Fix old sandboxes not being GC'ed issue
Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-933719487
Looks good, will you be sending a separate PR for the mesos tests?
Can you update the commit title / description to reflect the timeout library issue and not the mesos related issue? We treat libprocess / stout as separate libraries so try to avoid talking about mesos in their commits.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #410: Fix Timeout overflow issue caused by negative duration
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #410:
URL: https://github.com/apache/mesos/pull/410#discussion_r730092807
##########
File path: 3rdparty/libprocess/include/process/timeout.hpp
##########
@@ -34,10 +34,20 @@ class Timeout
// from now.
static Timeout in(const Duration& duration)
{
+ Time now = Clock::now();
+ if (duration < Duration::zero()) {
Review comment:
I'm a bit confused - why not simply return `now` if the duration is negative?
##########
File path: 3rdparty/libprocess/include/process/timeout.hpp
##########
@@ -34,10 +34,20 @@ class Timeout
// from now.
static Timeout in(const Duration& duration)
{
+ Time now = Clock::now();
+ if (duration < Duration::zero()) {
Review comment:
Alright I see, to preserve relative ordering of expired timeouts.
I guess that makes sense.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] bmahler commented on pull request #410: Fix old sandboxes not being GC'ed issue
Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #410:
URL: https://github.com/apache/mesos/pull/410#issuecomment-932461797
One more thing, we have to have PRs split across changes to stout/libprocess/mesos, so you'd need one patch for the timeout header and one for the mesos test.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org