Github user justsomeone1001 commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/702#issuecomment-181147188
I guess this isn't part of the 72-hour moratorium...
@Leemoonsoo
**Rscala** Actually its not just that rscala *can* be downloaded at build
time. It *will* be downloaded whenever the user updates their R package
library. R data scientists users do quite regularly because the functionality
is built into R. There is no way to prevent an updated rscala from being used
in place of the rscala included with this code. (The issue is the R-side of
the rscala library more than the java side, but that's beside the point.)
This has already created breakages: The author of rscala recently fixed a
bug that affected Windows users. The new version broke Eric's implementation,
which had to be refactored.
**Licenses** Regarding licenses --you are correct that rscala is available
under BSD. I convinced the author to switch from GPL to BSD so it could be used
in PR 208.
**Tests** You are astute to require unit tests. The reason this code is
passing CI is that it isn't being tested *at all.* Neither R, nor the classes
here, are ever executed.
Related to that: There's no code here to handle changes in the
SparkInterpreter class, which has caused severe breakages in the
spark-interpreter/R integration in the past. There's also no code in here to
handle the common Zeppelin/Spark misconfigurations that are 99% of the user
issues that arise when you add R to zeppelin; or to handle spark-related issues
at all.
One reason for this is, surely, because Eric only added the integration a
few weeks ago, after I pointed-out on the mailing list that the functionality
was absent. Since Eric based his code on mine (which he is absolutely entitled
to do), I'm pretty sure it will start to have the same issues if it ever
develops a user base.
**Other** There are a slew of other issues here related to integration with
the rest of Zeppelin, presentation of R in a manner that R-using data
scientists will be familiar with, and allowing the full R functionality.
As just one example: it doesn't support R base-plotting, and I'm pretty
sure it won't support interactive visualizations either.
I am not going to list all of those issues here now, when we're trying to
resolve the issue of PR 208 -- which *does* handle all of them, and quite a few
more. This is because I worked through them over the last six months in
responding to requests and issues from users.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
