hadoop-yarn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wangda Tan <wheele...@gmail.com>
Subject Re: [Discuss] Merge YARN-3368 to trunk
Date Thu, 22 Sep 2016 20:24:17 GMT
Hi Allen,

Thanks again for replying, see replies inline.

On Thu, Sep 22, 2016 at 11:52 AM, Allen Wittenauer <aw@effectivemachines.com
> wrote:

> > 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 tree?
> > 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.

Actually I'm not trying to debate about if it is necessary to make the UI
project following Hadoop's rules. The answer is always yes to me: we should
follow Hadoop's guide for any of sub project. We're trying to make it
follow Hadoop code style for the new sub project. For example, we use the
same indent rules (2 spaces) in all JS files.

But what I want to discuss is, does these files violate existing Hadoop
code style guide? I'm not be able to find rules states that dot-files or
files which are used by developing purpose should not be added to source
tree. Could you please share the link to the guide.

> > >* 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.

I would rather do this till we think it is ready to replace the entire
existing UI, otherwise it brings extra effort for existing contributors
building the code which they're not intended to use.

> >  >    * 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.

I will file a Yetus issue later, hopefully it should be a pretty simple
change. Could we enable profile for only specific Hadoop branch?

> > >* 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.

Yeah I agree :)


>         Thanks.

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message