Adds delaySeconds field to healthchecks#306
Adds delaySeconds field to healthchecks#306cihangirbesiktas wants to merge 4 commits intogambol99:masterfrom
Conversation
timoreimann
left a comment
timoreimann
left a comment
There was a problem hiding this comment.
Is this parameter documented somewhere? I can't find much about it.
health.go
Outdated
| GracePeriodSeconds: 30, | ||
| IntervalSeconds: 10, | ||
| TimeoutSeconds: 5, | ||
| DelaySeconds: 15, |
There was a problem hiding this comment.
I'd rather not extend the default health check unless there's a really good reason to. IMHO, users should pick proper values on their own.
There was a problem hiding this comment.
In marathon api https://mesosphere.github.io/marathon/api-console/index.html, you can find the parameter under Types->app.health.AppHealthCheck and the default value is 15. And we are using this variable in our DC/OS cluster.
There was a problem hiding this comment.
Thanks for the pointer.
Shouldn't Marathon pick the default if delaySeconds is omitted from the API request?
There was a problem hiding this comment.
yes, but we would like to be able to change it. And we could have some app definitions that include delaySeconds and we would like to support them.
There was a problem hiding this comment.
Sorry, I did not mean to say that we should not support the parameter (we definitely should). I'm just not sure if we should extend NewDefaultHealthCheck(). It looks like you could define a custom value for DelaySeconds by creating a health check (either an empty one or based off of NewDefaultHealthCheck) and set the value accordingly?
There was a problem hiding this comment.
I think Marathon API's default value of 15 which I give as a default in NewDefaultHealthCheck() should be allright.
There was a problem hiding this comment.
I would also prefer to not have it set by default. Without setting it in this client library here, we leave it to the Marathon server to change the default value without us requiring a change. AFAICS we don't loose anything through this, but gain a small little bit more resiliency against future Marathon updates. WDYT?
|
The tests are currently failing. |
residency.go
Outdated
| @@ -0,0 +1,35 @@ | |||
| /* | |||
| Copyright 2014 Rohith All rights reserved. | |||
|
Removed residency since it is on another PR and made the tests pass. Could you approve and merge @timoreimann ? |
|
@cihangirbesiktas as expressed by myself and @marco-jantke, extending If you think there's a good reason to keep it, I'd be grateful for an explanation. Otherwise, I'd prefer to see that part getting removed. |
|
We should have one small test somewhere that verifies we can properly deserialize the test data JSON that you have extended by the new field. |
luisdavim
force-pushed
the
master
branch
2 times, most recently
from
40c9cd1 to
c1d9ced
Compare
|
What kind of test are you expecting? |
3 participants
No description provided.