Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5144#issuecomment-86243643
Hi @tnachen thanks a lot for working on this. Mesos is the last mode that
does not support cluster mode and it would wonderful to see it reach parity on
this regard.
Overall I think the high level approach is reasonable, but there are a few
concerns I had about the code organization and style. In particular, there is a
lot of code to be reused between the Mesos and standalone submission gateways,
and you have refactored this nicely by making `RestServer` an abstract class.
However, there are many instances where the Mesos code just reaches into the
standalone packages to grab specific values or classes from there. A cleaner
way to refactor this is to abstract all common fields and classes that need to
be reused in common packages (even `o.a.s.deploy.` is probably fine).
The other thing is that this patch creates many small classes, most of
which I believe are unnecessary. Having many related small classes imposes
readability burden and makes the code much harder to follow. For instance, we
probably shouldn't follow YARN's favorite pattern of making an abstract parent
class that only has one implementation (e.g. `ContainerManagerImpl`) for
`MesosClusterScheduler`. I pointed out other examples inline (e.g.
`MesosDriverDescription`). IIUC the motivation behind some of these classes is
to follow closely the standalone mode behavior, but this is actually not a
requirement. In the worst case, this could lead to maintenance burden in the
future if we tie the implementation of the two modes together excessively.
Lastly, there are a lot of code style deviations from the rest of the code
base. Good references include the [Spark style
guide](https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide)
or the [Databricks style
guide](https://github.com/databricks/scala-style-guide), which was written
around writing code in Spark.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
