-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
auto hostname attribute #563
Conversation
51977dc
to
c84dbc6
Compare
so i validated that the above works with mesos the code currently grabs the not sure why that is or if it is configurable somewhere to have the offer contain a dns-name? does anyone with more experience in that area have any insight? i.e. this works for me
but this doesn't:
|
01fff3f
to
17ad52c
Compare
/** | ||
* Helper for checking resource offer against job constraints | ||
* | ||
* @author tony kerz ([email protected]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, i was just following what seemed like a prevailing convention...
@tony-kerz thanks a lot for this contribution. I posted some very minor comments, once you address them, I'll gladly merge this PR and include it in the upcoming release (2.5.0). Happy New Year! |
17ad52c
to
91407ed
Compare
@gkleiman thanks gaston, i tweaked the p/r per your suggestions. let me know if you want me to nuke the tbh, i was having some issues running the tests at all within eclipse, i also, may have been running with a more recent version of |
(i.e. the `hostname` attribute is not currently automatically available for constraints [as it is in Marathon](https://mesosphere.github.io/marathon/docs/constraints)) | ||
These constraints will work against attributes that are specifically set on the Mesos slaves [as described in the Mesos documentation](http://mesos.apache.org/documentation/latest/configuration). | ||
|
||
If a `hostname` attribute is not explicitly specified, one will automatically be created and made available for constraints [as it is in Marathon](https://mesosphere.github.io/marathon/docs/constraints). It should be noted that calling out specific hostnames is not resilient to slave failure and should be avoided if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid that this is not have Marathon behaves.
If you look at Marathon's Constraints.scala, you can see that it will always use the hostname taken from the hostname
field in the offer, even if the offer also includes a hostname
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, that sentence is ambiguous.
the aspect i was trying to liken to marathon is about hostname
automatically being made available for constraints.
good call tho, i'll try and re-word more specifically.
@tony-kerz thank you again for your contribution and for reacting so quickly to my previous comments. I am sorry it took so long to get your PR reviewed. I added some more comments about things I missed in the initial review. Regarding the tests, protos are immutable, so there should be no need for making the specification isolated. I checked out your branch and |
thanks for the scala tips @gkleiman, can you review this latest revision when you get the chance ✌️ |
oh, also, do you have any comment on the fact that i was only really able to use ip-addr as a constraint value vs an actual hostname? any idea if that is somehow alterable via mesos configuration, or just inherent? either way, i was thinking it should be mentioned in the documentation, cause i spent a cycle or two figuring out that i had to use an ip-addr v a hostname, and since the constraint key is |
@gkleiman is there anything else i need to do on this for it to be suitable for a merge? |
Using the IP in |
per #555