There were at least a couple of ideas behind the original thinking on using both of those Maps: 1) We needed the ability to efficiently get from jobId to both ActiveJob objects and to sets of associated Stages, and using both Maps here was an opportunity to do a little sanity checking to make sure that the Maps looked at least minimally consistent for the Job at issue; 2) Similarly, it could serve as a kind of hierarchical check -- first, for the Job which we are being asked to cancel, that we ever knew enough to even register its existence; second, that for a JobId that passes the first test, that we still have an ActiveJob that can be canceled.
Now, without doing a bunch of digging into the code archives, I can't tell you for sure whether those ideas were ever implemented completely correctly or whether they still make valid sense in the current code, but from looking at the lines that you highlighted, I can tell you that even if the ideas still make sense and are worth carrying forward, the current code doesn't implement them correctly. In particular, if it is possible for the `jobId` to not be in `jobIdToActiveJob`, we're going to produce a `NoSuchElementException` for the missing key instead of handling it or even reporting it in a more useful way. On Thu, Oct 13, 2016 at 8:11 AM, Jacek Laskowski <ja...@japila.pl> wrote: > Thanks Imran! Not only did the response come so promptly, but also > it's something I could work on (and have another Spark contributor > badge unlocked)! Thanks. > > Pozdrawiam, > Jacek Laskowski > ---- > https://medium.com/@jaceklaskowski/ > Mastering Apache Spark 2.0 https://bit.ly/mastering-apache-spark > Follow me at https://twitter.com/jaceklaskowski > > > On Thu, Oct 13, 2016 at 5:02 PM, Imran Rashid <iras...@cloudera.com> > wrote: > > Hi Jacek, > > > > doesn't look like there is any good reason -- Mark Hamstra might know > this > > best. Feel free to open a jira & pr for it, you can ping Mark, Kay > > Ousterhout, and me (@squito) for review. > > > > Imran > > > > On Thu, Oct 13, 2016 at 7:56 AM, Jacek Laskowski <ja...@japila.pl> > wrote: > >> > >> Hi, > >> > >> Is there a reason why DAGScheduler.handleJobCancellation checks the > >> active job id in jobIdToStageIds [1] while looking the job up in > >> jobIdToActiveJob [2]? Perhaps synchronized earlier yet still > >> inconsistent. > >> > >> [1] > >> https://github.com/apache/spark/blob/master/core/src/ > main/scala/org/apache/spark/scheduler/DAGScheduler.scala#L1372 > >> [2] > >> https://github.com/apache/spark/blob/master/core/src/ > main/scala/org/apache/spark/scheduler/DAGScheduler.scala#L1376 > >> > >> Pozdrawiam, > >> Jacek Laskowski > >> ---- > >> https://medium.com/@jaceklaskowski/ > >> Mastering Apache Spark 2.0 https://bit.ly/mastering-apache-spark > >> Follow me at https://twitter.com/jaceklaskowski > >> > >> --------------------------------------------------------------------- > >> To unsubscribe e-mail: dev-unsubscr...@spark.apache.org > >> > > >
