Guys, While catching up on my emails from the last a couple of weeks, this thread caught my attention. I am not normally paying much attention to the code reviews traffic from GH, but this one is pretty different as it spans three months and counting.
First, here are my five cents:
- r/R/rzeppelin/LICENSE is wrong: if the code is aimed to be contributed to
an ASF project this file should simply contain ASL2 text, like in [1]
- r/pom.xml perhaps shouldn't contain a separate <developers> section, but
Zeppelin might have different guidelines on it. Say, Bigtop doesn't
maintain this in the source code, but we have the list of all the
committers on the project's site [2] Every new committer's first commit is
to update the team page ;)
- comments like in r/src/main/java/org/apache/zeppelin/rinterpreter/KnitR.java
+/**
+ * Created by aelberg on 7/28/15.
+ */
is better to be removed. It has been already discussed here [3]. And the
initial discussion on the topic [4] was linked as well
- same goes to r/R/rzeppelin/DESCRIPTION. I am not sure if this is R-specific
stuff - I have no idea about R, honestly, but
+License: GPL (>= 2) | BSD_3_clause + file LICENSE
...
+Author: David B. Dahl
shouldn't be here, IMO. Normally, if some additional licenses are used,
they have to be listed in the top-level NOTICE file (already there).
- I am not going to offer any comments on the technical merits of the patch,
as I haven't tried it personally. However, I don't see any serious
technical objections to the functionality in question.
So, the question is - why the PR is open for three months? I hasn't been able
to get a clear answer. What I found out though is pretty unsettling, really.
The communication on the topic is almost non-existent, except for this sparse
and bitter thread in the GH.
I would love to hear from the committers what's stopping the acceptance of the
code, besides of the minor issues I've mentioned earlier? What are the reasons
for it?
Is there anything wrong with it?
One of the responsibilities of the committers is to make sure the
contributions are reviewed; new contributors are guided and do understand how
the project ticks. The easy feedback flow attracts new people, allowing the
community to grow and thrive.
I am asking _explicitely_ not to re-start the bickering I have already
seen. At this point I am interested in the purely technical side of this.
[1] https://github.com/apache/bigtop/blob/master/LICENSE
[2] http://bigtop.apache.org/team-list.html
[3]
http://apache-nifi-developer-list.39713.n7.nabble.com/author-tags-td1335.html
[4] http://s.apache.org/iZl
With regards,
Cos
On Mon, Nov 16, 2015 at 11:06PM, elbamos wrote:
> Github user elbamos commented on the pull request:
>
>
> https://github.com/apache/incubator-zeppelin/pull/208#issuecomment-157203411
>
> The current push should resolve some issues with changes in the
> Spark-Zeppelin interface that had created issues for users, as well as
> support for 1.5.1.
>
> Knitr support is improved, and the reason for a separate knitr
> interpreter may be clearer now.
>
> The amount of code borrowed from rscala is reduced.
>
> I did not address issues with @author tags, or files under the R/
> folder. The reason is, to be blunt, I don't understand what the precise
> concerns actually are.
>
> Please note that I changed .travis.yml to only use spark 1.4 and later.
> I'm sure there is a better way to do it, but I'm just not in a position
> to try to figure out the entire Zeppelin build process.
>
> Integrating this with the rest of zeppelin is going to take some work
> regarding pom's, travis, and so forth. I can do a lot of that, but I'm
> going to need to discuss it with the people who have been "owning" those
> files. There are just too many moving pieces here.
>
> Because of the size of this (which is, unfortunately, necessary),
> posting here is probably not the most efficient way. That is also true
> because certain people chose to use this PR as a forum to air other
> issues. Therefore, it would be better if reviewers e-mail me directly.
signature.asc
Description: Digital signature
