You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Tom Beerbower <tb...@hortonworks.com> on 2015/05/25 22:43:47 UTC

Review Request 34657: Views : expose extra classpath setting

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/
-----------------------------------------------------------

Review request for Ambari, Jonathan Hurley and Nate Cole.


Bugs: AMBARI-11208
    https://issues.apache.org/jira/browse/AMBARI-11208


Repository: ambari


Description
-------

Each view should expose a way to set extra classpath info for a view.

    <view>
      <name>HELLO_WORLD</name>
      <label>The Hello World View!</label>
      <version>1.0.0</version>
      <classpath>
        <path>/usr/lib/ambari-server/foo.jar</path>
        <path>/home/hdfs/</path>
       </classpath>
       <instance>
         <name>INSTANCE_1</name>
       </instance>
    </view>


Diffs
-----

  ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
  ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
  ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
  ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
  ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
  ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
  ambari-views/src/main/resources/view.xsd 37737c1 

Diff: https://reviews.apache.org/r/34657/diff/


Testing
-------

Manual tested.

Added new unit tests.

mvn clean test

All tests pass.


Thanks,

Tom Beerbower

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85188
-----------------------------------------------------------

Ship it!


Ship It!

- Nate Cole


On May 26, 2015, 12:03 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 12:03 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85191
-----------------------------------------------------------

Ship it!


Ship It!

- Jonathan Hurley


On May 26, 2015, 12:03 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 12:03 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Tom Beerbower <tb...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/
-----------------------------------------------------------

(Updated May 26, 2015, 4:03 p.m.)


Review request for Ambari, Jonathan Hurley and Nate Cole.


Changes
-------

updated diff


Bugs: AMBARI-11208
    https://issues.apache.org/jira/browse/AMBARI-11208


Repository: ambari


Description
-------

Each view should expose a way to set extra classpath info for a view.

    <view>
      <name>HELLO_WORLD</name>
      <label>The Hello World View!</label>
      <version>1.0.0</version>
      <classpath>
        <path>/usr/lib/ambari-server/foo.jar</path>
        <path>/home/hdfs/</path>
       </classpath>
       <instance>
         <name>INSTANCE_1</name>
       </instance>
    </view>


Diffs (updated)
-----

  ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
  ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
  ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
  ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
  ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
  ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
  ambari-views/src/main/resources/view.xsd 37737c1 

Diff: https://reviews.apache.org/r/34657/diff/


Testing
-------

Manual tested.

Added new unit tests.

mvn clean test

All tests pass.


Thanks,

Tom Beerbower

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Tom Beerbower <tb...@hortonworks.com>.

> On May 26, 2015, 3:21 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java, line 260
> > <https://reviews.apache.org/r/34657/diff/1/?file=971506#file971506line260>
> >
> >     Comma-separated classpath? Does that work on all OSes?

Thanks for the review.  I used a comma separated list because that is accepted by Jetty WebAppContext.setExtraClasspath() which is what gets called in the ViewClassLoader.  Doesn't matter what the OS is.


- Tom


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85177
-----------------------------------------------------------


On May 25, 2015, 8:43 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 25, 2015, 8:43 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Jonathan Hurley <jh...@hortonworks.com>.

> On May 26, 2015, 11:21 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java, line 260
> > <https://reviews.apache.org/r/34657/diff/1/?file=971506#file971506line260>
> >
> >     Comma-separated classpath? Does that work on all OSes?
> 
> Tom Beerbower wrote:
>     Thanks for the review.  I used a comma separated list because that is accepted by Jetty WebAppContext.setExtraClasspath() which is what gets called in the ViewClassLoader.  Doesn't matter what the OS is.

Gotcha; OK, then just the StringUtils change that was mentioned in the first review.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85177
-----------------------------------------------------------


On May 25, 2015, 4:43 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 25, 2015, 4:43 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85177
-----------------------------------------------------------

Ship it!



ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java
<https://reviews.apache.org/r/34657/#comment136688>

    Comma-separated classpath? Does that work on all OSes?


- Jonathan Hurley


On May 25, 2015, 4:43 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 25, 2015, 4:43 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Tom Beerbower <tb...@hortonworks.com>.

> On May 26, 2015, 2:58 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java, lines 252-267
> > <https://reviews.apache.org/r/34657/diff/1/?file=971506#file971506line252>
> >
> >     Nit: can use StringUtils.join here to make your life easier :)

Thanks for the review.  That makes it nice and clean.  I'll update.


- Tom


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85174
-----------------------------------------------------------


On May 25, 2015, 8:43 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 25, 2015, 8:43 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>

Re: Review Request 34657: Views : expose extra classpath setting

Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34657/#review85174
-----------------------------------------------------------

Ship it!



ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java
<https://reviews.apache.org/r/34657/#comment136685>

    Nit: can use StringUtils.join here to make your life easier :)


- Nate Cole


On May 25, 2015, 4:43 p.m., Tom Beerbower wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34657/
> -----------------------------------------------------------
> 
> (Updated May 25, 2015, 4:43 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and Nate Cole.
> 
> 
> Bugs: AMBARI-11208
>     https://issues.apache.org/jira/browse/AMBARI-11208
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Each view should expose a way to set extra classpath info for a view.
> 
>     <view>
>       <name>HELLO_WORLD</name>
>       <label>The Hello World View!</label>
>       <version>1.0.0</version>
>       <classpath>
>         <path>/usr/lib/ambari-server/foo.jar</path>
>         <path>/home/hdfs/</path>
>        </classpath>
>        <instance>
>          <name>INSTANCE_1</name>
>        </instance>
>     </view>
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewClassLoader.java b7cb594 
>   ambari-server/src/main/java/org/apache/ambari/server/view/ViewExtractor.java 43efc7d 
>   ambari-server/src/main/java/org/apache/ambari/server/view/configuration/ViewConfig.java 6164bb7 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewClassLoaderTest.java 8e22c49 
>   ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java a0ee6b3 
>   ambari-server/src/test/java/org/apache/ambari/server/view/configuration/ViewConfigTest.java beb8bde 
>   ambari-views/src/main/resources/view.xsd 37737c1 
> 
> Diff: https://reviews.apache.org/r/34657/diff/
> 
> 
> Testing
> -------
> 
> Manual tested.
> 
> Added new unit tests.
> 
> mvn clean test
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Tom Beerbower
> 
>