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/11/29 13:47:32 UTC
[GitHub] [mesos] Lqp1 opened a new pull request #415: Explicitely fail content length fetch in case of error
Lqp1 opened a new pull request #415:
URL: https://github.com/apache/mesos/pull/415
Otherwise we used to use the size of various error message instead of
the artifact size.
This will make Mesos more robust, but it will also mean artifacts fetch
might fail more in case inconsistencies
--
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] andreaspeters commented on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-988962972
@cf-natali for me it looks fine and with the response code handling more "nice" then before. :+1:
--
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] Lqp1 edited a comment on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
Lqp1 edited a comment on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-993308012
Hello :)
Thanks for your feedback.
> the file could be modified between the HEAD and the GET.
Yes, this is why there is a mechanism inside Mesos to adapt the cache following the GET.
To me it's weirder to try to allocate space following a "bad" HEAD request response than failing directly.
I don't think there is a higher risk of breakage with it, but I'd understand you prefer to ignore this PR as it changes how Mesos behaves. We can also make this change more specific by raising in case of 404 and 500 errors for example (which are the most common ones). Just FYI, we are now running this patch in production, without any issue so far.
--
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 closed pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
cf-natali closed pull request #415:
URL: https://github.com/apache/mesos/pull/415
--
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] andreaspeters commented on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-988939467
@cf-natali @qianzhangxa can u help here?
--
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 #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-990392160
Hey,
I need to think about this a bit ore, I'm a bit concerned about the potential non backward-compatibility.
--
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] andreaspeters commented on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-992960952
Ok, I understand. More risk then we can win makes also no sense for me. But in these case, I think we should close these PR. :-)
--
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] Lqp1 edited a comment on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
Lqp1 edited a comment on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-993308012
Hello :)
Thanks for your feedback.
> the file could be modified between the HEAD and the GET.
Yes, this is why there is a mechanism inside Mesos to adapt the cache following the GET.
To me it's weirder to try to allocate space following a "bad" HEAD request response than failing directly.
I don't think there is a higher risk of breakage with it, but I'd understand you prefer to ignore this PR as it changes how Mesos behaves. We can also make this change more specific by raising in case of 404 and 500 errors for example (which are the most common ones). Just FYI, we are now running this patch in production, without any issue so far.
--
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] Lqp1 commented on pull request #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-993308012
Hello :)
Thanks for your feedback.
> the file could be modified between the HEAD and the GET.
--
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 #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-992762929
One possibility is the server returning the correct `Content-Length`, but returning a non-`200` status.
It'd be a bit weird but who knows.
Also, as explained by @Lqp1, the following `GET` will fail anyway in case of error - and generally relying on `HEAD`/`Content-Length` is fragile since it's not atomic: the file could be modified between the `HEAD` and the `GET`.
So given the risk of breakage, and the limited impact since the downstream problem has been fixed, I'd be tempted to ignore.
--
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 #415: Explicitely fail content length fetch in case of error
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #415:
URL: https://github.com/apache/mesos/pull/415#issuecomment-996982882
> Just FYI, we are now running this patch in production, without any issue so far.
Yes - I'm sure it works as intended in your environment, the problem is guaranteeing it will won't regress anywhere :).
I'm closing, but thanks for the contribution!
--
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