hadoop-yarn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Allen Wittenauer ...@effectivemachines.com>
Subject Re: [Discuss] Merge YARN-3368 to trunk
Date Thu, 22 Sep 2016 18:52:44 GMT

> On Sep 22, 2016, at 11:13 AM, Wangda Tan <wheeleast@gmail.com> wrote:
> >* why are there editor settings and other superfluous things there?  do these settings
comply with the PMC's style guide?  why would they be here and not in the base of the source

> Actually many front-end projects are include these files in their source tree. I think
the major reason is front end development needs different toolchains comparing to Java development.

	That's fine for those projects, but we're talking about this project--Apache Hadoop. We strive
(and even sometimes achieve) code consistency throughout the entire project.  It's so important
that it's even codified in the committer guidelines.  Why should one chunk of code--in a sub-project,
and a currently optional component at that--have settings different (?) than the rest of the
code base?   Having potentially different formatting rules is a direct hinderance to others
looking at and contributing to the code.

	In order to respect the guidelines established by the PMC, it seems these either need to
get moved to the root of the source tree and/or made consistent with the rest of the code
or deleted.

> >* I'm still not sure why this is being wrapped in a maven profile:
> >    * create-release is activating the profile, so why not enable it for everyone?
> The major reason is, create-release run using docker image, all necessary dependencies
are already included. But we don't expect every developer install npm and all the other dependencies
to build the UI code.
> This is similar to native code, user who is interested to modify and repackage it can
enable the profile and build, but I don't expect everybody is required to do that.

	This is a good point. However,  if the intent is to make this the default UI, then it's better
to bite the bullet and put these requirements in place now while 3.x is in alpha rather than
flip the switch later.

>  >    * precommit won't test it with that profile in place; it'd be useful to see
a *real* run of yetus against this branch without the maven profile protecting it
> Actually this is also what I want to do, I'm not quite sure what need to do to enable
profile while running the precommit, if it is possible, could you point me which code I need
to update?

	I grouped these two bullet points intentionally: with the exception of some documentation
bits, create-release is specifically coded currently to build what precommit tests. (See,
for example, ISA-L support). Turning this on for create-release without precommit support
is premature.  You need to get a Yetus issue filed to turn on that profile first.

	It's worthwhile pointing out that without this support in place, this is basically a discussion
about merging a branch that has very little precommit testing in place specifically because
of the maven profile.

> >* this doesn't appear to actually build in target/, which is very counter to maven.
is there a valid reason for that?
> I will double check it, I just found bower supports to build in a separate folder, but
not quite sure about npm. The bottom line is we should be able to copy the whole folder to
target and build it.

	Awesome.  I recognize this is a pain, but maven really really really really really doesn't
like things outside of target/.	We've got some other things like this to fix, but we really
need to work on making more of them.

To unsubscribe, e-mail: yarn-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-dev-help@hadoop.apache.org

View raw message