{"action":"created","issue":{"url":"https://api.github.com/repos/apache/spark/issues/9164","labels_url":"https://api.github.com/repos/apache/spark/issues/9164/labels{/name}","comments_url":"https://api.github.com/repos/apache/spark/issues/9164/comments","events_url":"https://api.github.com/repos/apache/spark/issues/9164/events","html_url":"https://github.com/apache/spark/pull/9164","id":112076491,"number":9164,"title":"[SPARK-11178] Improving naming around task failures.","user":{"login":"kayousterhout","id":1108612,"avatar_url":"https://avatars.githubusercontent.com/u/1108612?v=3","gravatar_id":"","url":"https://api.github.com/users/kayousterhout","html_url":"https://github.com/kayousterhout","followers_url":"https://api.github.com/users/kayousterhout/followers","following_url":"https://api.github.com/users/kayousterhout/following{/other_user}","gists_url":"https://api.github.com/users/kayousterhout/gists{/gist_id}","starred_url":"https://api.github.com/users/kayousterhout/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kayousterhout/subscriptions","organizations_url":"https://api.github.com/users/kayousterhout/orgs","repos_url":"https://api.github.com/users/kayousterhout/repos","events_url":"https://api.github.com/users/kayousterhout/events{/privacy}","received_events_url":"https://api.github.com/users/kayousterhout/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2015-10-19T05:42:44Z","updated_at":"2015-10-26T05:17:27Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/apache/spark/pulls/9164","html_url":"https://github.com/apache/spark/pull/9164","diff_url":"https://github.com/apache/spark/pull/9164.diff","patch_url":"https://github.com/apache/spark/pull/9164.patch"},"body":"Commit af3bc59d1f5d9d952c2d7ad1af599c49f1dbdaf0 introduced new\r\nfunctionality so that if an executor dies for a reason that's not\r\ncaused by one of the tasks running on the executor (e.g., due to\r\npre-emption), Spark doesn't count the failure towards the maximum\r\nnumber of failures for the task.  That commit introduced some vague\r\nnaming that this commit attempts to fix; in particular:\r\n\r\n(1) The variable \"isNormalExit\", which was used to refer to cases where\r\nthe executor died for a reason unrelated to the tasks running on the\r\nmachine, has been renamed to \"exitUnrelatedToRunningTasks\". The problem\r\nwith the existing name is that it's not clear (at least to me!) what it\r\nmeans for an exit to be \"normal\"; the new name is intended to make the\r\npurpose of this variable more clear.\r\n\r\n(2) The variable \"shouldEventuallyFailJob\" has been renamed to\r\n\"countTowardsTaskFailures\". This variable is used to determine whether\r\na task's failure should be counted towards the maximum number of failures\r\nallowed for a task before the associated Stage is aborted. The problem\r\nwith the existing name is that it can be confused with implying that\r\nthe task's failure should immediately cause the stage to fail because it\r\nis somehow fatal (this is the case for a fetch failure, for example: if\r\na task fails because of a fetch failure, there's no point in retrying,\r\nand the whole stage should be failed).\r\n\r\nThere are a couple of remaining issues that it would be helpful if\r\nfolks commented on:\r\n\r\n(a) In YarnAllocator.scala, line 447: what does \"exited normally\" mean\r\nhere? I'm hoping to improve the comment to be clearer to those less\r\nfamiliar with YARN.\r\n\r\n(b) In SparkDeploySchedulerBackend, the original commit seems to have\r\nchanged the behavior here so that when executorRemoved is called, the\r\nfailure isn't counted towards the task's maximum number of failures.\r\nWhy is this? It looks like executorRemoved can be called because\r\nthe JVM died, which can be the result of an issue with a running task?\r\n\r\n(c) This commit changes the JsonProtocol. I think this is OK because the\r\nexisting Json was never part of a release, so this only affects folks\r\nwho run on master, and it just means that if someone uses the new\r\ncode to load old event logs, the value of \"isNormalExit\" i.e.\r\n\"exitUnrelatedToRunningTasks\" won't display correctly, but it would be\r\ngreat if others could verify this.\r\n\r\n(d) I don't love using a negative in the variable name\r\n(\"exitUnrelatedToRunningTasks\") because it leads to a double-negative\r\nwhen the variable is false.  But I also thought this might be more clear\r\nthan a variable called \"exitRelatedToRunningTasks\", because with the latter\r\nname, it seems like it should always be true.  More importantly, I didn't\r\nwnt to reverse the variable unless others agreed it should be done, because\r\nit's annoying to reverse all of the uses of it.  Let me know if you think it\r\nshould be reversed, and I'll do that.\r\n\r\n(e) For the case where the commit failed, we never count that towards the\r\nmax failures.  Is there any legimate reason the commit could fail? I'm\r\nwondering if it's possible to have a stage that fails infinitely many times\r\ndue to a real problem with committing the task.\r\n\r\ncc @andrewor14 @mccheah and @vanzin who were involved in the original naming"},"comment":{"url":"https://api.github.com/repos/apache/spark/issues/comments/151027212","html_url":"https://github.com/apache/spark/pull/9164#issuecomment-151027212","issue_url":"https://api.github.com/repos/apache/spark/issues/9164","id":151027212,"user":{"login":"SparkQA","id":8108735,"avatar_url":"https://avatars.githubusercontent.com/u/8108735?v=3","gravatar_id":"","url":"https://api.github.com/users/SparkQA","html_url":"https://github.com/SparkQA","followers_url":"https://api.github.com/users/SparkQA/followers","following_url":"https://api.github.com/users/SparkQA/following{/other_user}","gists_url":"https://api.github.com/users/SparkQA/gists{/gist_id}","starred_url":"https://api.github.com/users/SparkQA/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SparkQA/subscriptions","organizations_url":"https://api.github.com/users/SparkQA/orgs","repos_url":"https://api.github.com/users/SparkQA/repos","events_url":"https://api.github.com/users/SparkQA/events{/privacy}","received_events_url":"https://api.github.com/users/SparkQA/received_events","type":"User","site_admin":false},"created_at":"2015-10-26T05:17:27Z","updated_at":"2015-10-26T05:17:27Z","body":"  [Test build #44338 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44338/consoleFull) for   PR 9164 at commit [`fc79278`](https://github.com/apache/spark/commit/fc792785d408914a51811d9c46f6da07bfb8fa9e)."}}