{"action":"created","issue":{"url":"https://api.github.com/repos/mesosphere/marathon/issues/4210","repository_url":"https://api.github.com/repos/mesosphere/marathon","labels_url":"https://api.github.com/repos/mesosphere/marathon/issues/4210/labels{/name}","comments_url":"https://api.github.com/repos/mesosphere/marathon/issues/4210/comments","events_url":"https://api.github.com/repos/mesosphere/marathon/issues/4210/events","html_url":"https://github.com/mesosphere/marathon/pull/4210","id":170709572,"number":4210,"title":"Fixes #4168 - Add support for Mesos HTTP/TCP health checks","user":{"login":"gkleiman","id":146374,"avatar_url":"https://avatars.githubusercontent.com/u/146374?v=3","gravatar_id":"","url":"https://api.github.com/users/gkleiman","html_url":"https://github.com/gkleiman","followers_url":"https://api.github.com/users/gkleiman/followers","following_url":"https://api.github.com/users/gkleiman/following{/other_user}","gists_url":"https://api.github.com/users/gkleiman/gists{/gist_id}","starred_url":"https://api.github.com/users/gkleiman/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gkleiman/subscriptions","organizations_url":"https://api.github.com/users/gkleiman/orgs","repos_url":"https://api.github.com/users/gkleiman/repos","events_url":"https://api.github.com/users/gkleiman/events{/privacy}","received_events_url":"https://api.github.com/users/gkleiman/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/mesosphere/marathon/labels/ready%20for%20review","name":"ready for review","color":"ededed"}],"state":"open","locked":false,"assignee":{"login":"aquamatthias","id":330485,"avatar_url":"https://avatars.githubusercontent.com/u/330485?v=3","gravatar_id":"","url":"https://api.github.com/users/aquamatthias","html_url":"https://github.com/aquamatthias","followers_url":"https://api.github.com/users/aquamatthias/followers","following_url":"https://api.github.com/users/aquamatthias/following{/other_user}","gists_url":"https://api.github.com/users/aquamatthias/gists{/gist_id}","starred_url":"https://api.github.com/users/aquamatthias/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aquamatthias/subscriptions","organizations_url":"https://api.github.com/users/aquamatthias/orgs","repos_url":"https://api.github.com/users/aquamatthias/repos","events_url":"https://api.github.com/users/aquamatthias/events{/privacy}","received_events_url":"https://api.github.com/users/aquamatthias/received_events","type":"User","site_admin":false},"assignees":[{"login":"aquamatthias","id":330485,"avatar_url":"https://avatars.githubusercontent.com/u/330485?v=3","gravatar_id":"","url":"https://api.github.com/users/aquamatthias","html_url":"https://github.com/aquamatthias","followers_url":"https://api.github.com/users/aquamatthias/followers","following_url":"https://api.github.com/users/aquamatthias/following{/other_user}","gists_url":"https://api.github.com/users/aquamatthias/gists{/gist_id}","starred_url":"https://api.github.com/users/aquamatthias/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aquamatthias/subscriptions","organizations_url":"https://api.github.com/users/aquamatthias/orgs","repos_url":"https://api.github.com/users/aquamatthias/repos","events_url":"https://api.github.com/users/aquamatthias/events{/privacy}","received_events_url":"https://api.github.com/users/aquamatthias/received_events","type":"User","site_admin":false},{"login":"jasongilanfarr","id":794214,"avatar_url":"https://avatars.githubusercontent.com/u/794214?v=3","gravatar_id":"","url":"https://api.github.com/users/jasongilanfarr","html_url":"https://github.com/jasongilanfarr","followers_url":"https://api.github.com/users/jasongilanfarr/followers","following_url":"https://api.github.com/users/jasongilanfarr/following{/other_user}","gists_url":"https://api.github.com/users/jasongilanfarr/gists{/gist_id}","starred_url":"https://api.github.com/users/jasongilanfarr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jasongilanfarr/subscriptions","organizations_url":"https://api.github.com/users/jasongilanfarr/orgs","repos_url":"https://api.github.com/users/jasongilanfarr/repos","events_url":"https://api.github.com/users/jasongilanfarr/events{/privacy}","received_events_url":"https://api.github.com/users/jasongilanfarr/received_events","type":"User","site_admin":false},{"login":"jeschkies","id":2418440,"avatar_url":"https://avatars.githubusercontent.com/u/2418440?v=3","gravatar_id":"","url":"https://api.github.com/users/jeschkies","html_url":"https://github.com/jeschkies","followers_url":"https://api.github.com/users/jeschkies/followers","following_url":"https://api.github.com/users/jeschkies/following{/other_user}","gists_url":"https://api.github.com/users/jeschkies/gists{/gist_id}","starred_url":"https://api.github.com/users/jeschkies/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jeschkies/subscriptions","organizations_url":"https://api.github.com/users/jeschkies/orgs","repos_url":"https://api.github.com/users/jeschkies/repos","events_url":"https://api.github.com/users/jeschkies/events{/privacy}","received_events_url":"https://api.github.com/users/jeschkies/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2016-08-11T18:10:13Z","updated_at":"2016-08-30T15:17:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mesosphere/marathon/pulls/4210","html_url":"https://github.com/mesosphere/marathon/pull/4210","diff_url":"https://github.com/mesosphere/marathon/pull/4210.diff","patch_url":"https://github.com/mesosphere/marathon/pull/4210.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/mesosphere/marathon/4210)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/mesosphere/marathon/issues/comments/243475098","html_url":"https://github.com/mesosphere/marathon/pull/4210#issuecomment-243475098","issue_url":"https://api.github.com/repos/mesosphere/marathon/issues/4210","id":243475098,"user":{"login":"jasongilanfarr","id":794214,"avatar_url":"https://avatars.githubusercontent.com/u/794214?v=3","gravatar_id":"","url":"https://api.github.com/users/jasongilanfarr","html_url":"https://github.com/jasongilanfarr","followers_url":"https://api.github.com/users/jasongilanfarr/followers","following_url":"https://api.github.com/users/jasongilanfarr/following{/other_user}","gists_url":"https://api.github.com/users/jasongilanfarr/gists{/gist_id}","starred_url":"https://api.github.com/users/jasongilanfarr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jasongilanfarr/subscriptions","organizations_url":"https://api.github.com/users/jasongilanfarr/orgs","repos_url":"https://api.github.com/users/jasongilanfarr/repos","events_url":"https://api.github.com/users/jasongilanfarr/events{/privacy}","received_events_url":"https://api.github.com/users/jasongilanfarr/received_events","type":"User","site_admin":false},"created_at":"2016-08-30T15:17:44Z","updated_at":"2016-08-30T15:17:44Z","body":"\n\n\n\nReview status: 0 of 36 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.\n\n---\n\n*[project/build.scala, line 150 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQQyFAXeI7SYGA9bbCU:-KQQyFAXeI7SYGA9bbCV:bu3z297) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/project/build.scala#L150)):*\n> ```Scala\n>       \"Mesosphere Public Repo\"    at \"http://downloads.mesosphere.com/maven\",\n>       \"Typesafe Releases\" at \"http://repo.typesafe.com/typesafe/releases/\",\n>       \"Local Maven Repository\" at \"file://\"+Path.userHome.absolutePath+\"/.m2/repository\",\n> ```\n\nCan you mark a TODO for yourself?\n\n---\n\n*[src/main/scala/mesosphere/marathon/api/v2/json/AppUpdate.scala, line 57 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQQyLetOfKMofNQTQq_:-KQQyLetOfKMofNQTQqa:bwsr2ml) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/api/v2/json/AppUpdate.scala#L57)):*\n> ```Scala\n>     container: Option[Container] = None,\n> \n>     healthChecks: Option[Set[_ <: HealthCheck]] = None,\n> ```\n\nIsn't Set already Covariant? e.g. no need for a \"_ <:\"?\n\n---\n\n*[src/main/scala/mesosphere/marathon/api/v2/json/Formats.scala, line 13 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQQyRwQDHkssQnTCmyc:-KQQyRwQDHkssQnTCmyd:bc43bm4) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/api/v2/json/Formats.scala#L13)):*\n> ```Scala\n> import mesosphere.marathon.core.task.Task\n> import mesosphere.marathon.core.event._\n> import mesosphere.marathon.core.health._\n> ```\n\nCan you increase your import limits? I thought we were trying to avoid wildcard imports unless strictly necessary\n\n---\n\n*[src/main/scala/mesosphere/marathon/api/v2/json/Formats.scala, line 978 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQQywxdWwzSw2MzPMgy:-KQQywxe760l2HQlGFlH:b8mroj1) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/api/v2/json/Formats.scala#L978)):*\n> ```Scala\n>     val portIndexesMakeSense = app.portDefinitions.nonEmpty || hasPortMappings\n> \n>     if (portIndexesMakeSense) app.copy(healthChecks = addHealthCheckPortIndexIfNecessary(app.healthChecks))\n> ```\n\nShould we log if it doesn't make sense?\n\n---\n\n*[src/main/scala/mesosphere/marathon/core/health/HealthCheck.scala, line 12 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQQz2nBQibiexTg-0mT:-KQQz2nCtRknk3NsO24u:b5ks0yc) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/core/health/HealthCheck.scala#L12)):*\n> ```Scala\n> import scala.concurrent.duration._\n> \n> sealed trait HealthCheck extends MarathonState[Protos.HealthCheckDefinition, HealthCheck] {\n> ```\n\nWhy does this need to extend MarathonState? My impression is we're ultimately trying to get rid of this.\n\n---\n\n*[src/main/scala/mesosphere/marathon/core/health/HealthCheck.scala, line 333 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQR0Ebb3iQ6IZVrX_xD:-KQR0Ebb3iQ6IZVrX_xE:bq9c1ol) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/core/health/HealthCheck.scala#L333)):*\n> ```Scala\n>   }\n> \n>   def mergeFromProto(proto: Protos.HealthCheckDefinition): HealthCheck = {\n> ```\n\nI really don't like this method name. it really should be \"fromProto\" unless its going to read the current object and update only _some_ fields.\n\n---\n\n*[src/main/scala/mesosphere/marathon/state/RunSpec.scala, line 56 \\[r2\\]](https://reviewable.io:443/reviews/mesosphere/marathon/4210#-KQR0d_kxnYB3sv0szhk:-KQR0d_kxnYB3sv0szhl:b-kylm6b) ([raw file](https://github.com/mesosphere/marathon/blob/79d66ac37b21cf6afcaf22eb3b71f7234b07fa5b/src/main/scala/mesosphere/marathon/state/RunSpec.scala#L56)):*\n> ```Scala\n>   def container: Option[Container]\n> \n>   def healthChecks: Set[_ <: HealthCheck]\n> ```\n\nJust HealthCheck should be sufficient.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/mesosphere/marathon/4210)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"jasongilanfarr"}}