Peer Review of Embedded, ShrinkWrap and Bootstrap

[alrubinger]

Jesper was kind enough to have a look at our work and give his input.

"Jesper Pedesen" wrote:

Overall:
========

* Good idea to add overview.html / package.html
* Good idea to run checkstyle
* Good idea to run findbugs (with reportLevel="low")

Bootstrap:
==========
* api-embedded
* impl-embedded

- Needs to be move to Embedded - as they are a specific use-case of Bootstrap
- Refactor package names

* I'm not too crazy about BootstrapMetadata being in the spi/ package
* Also there are constants in spi/ which may not have a relevance for the actual environment

* I guess it is ok to keep the MC and AS specific modules here - as many projects will use them

* Missing excludes in pom.xml for MC and AS

org.jboss.bootstrap.api.server.Server
-------------------------------------
void registerEventHandler(LifecycleState state, LifecycleEventHandler handler) throws IllegalArgumentException;

arguments should be reversed to follow other registerEventHandler methods

org.jboss.bootstrap.api.lifecycle.LifecycleState
------------------------------------------------

Are we sure that there won't be any additional server states in the future ?
Or that an implementation will define additional states ?

In those cases an enum won't work - as it can't be extended.

Embedded:
=========

* Missing excludes in pom.xml

org.jboss.bootstrap.api.embedded.server.JBossASEmbeddedServerFactory
--------------------------------------------------------------------

"public static final ..." -> "private static final ..."

... otherwise a

createServer(final ClassLoader cl, final String className)

method is needed...


ShrinkWrap:
===========

- Check ContextClassLoader assumptions

org.jboss.shrinkwrap.api.Path
-----------------------------

* Would a org.jboss.cache.Fqn<E> style be better ?

org.jboss.shrinkwrap.api.Archive
--------------------------------

T add(Path target, String name, Asset asset) throws IllegalArgumentException;

switch assert and name

* Remove all "String target" -- Path should be used

T merge(Path path, Archive<?> source) throws IllegalArgumentException;

switch path and source


org.jboss.shrinkwrap.api.spec.FactoryUtil
-----------------------------------------

* createInstance method with ClassLoader

org.jboss.shrinkwrap.api.container.ResourceContainer
----------------------------------------------------

* Alignment of method arguments

org.jboss.shrinkwrap.api.container.EnterpriseContainer
------------------------------------------------------

* Alignment of method arguments

org.jboss.shrinkwrap.api.container.WebContainer
-----------------------------------------------

* Alignment of method arguments

org.jboss.shrinkwrap.api.container.ManifestContainer
----------------------------------------------------

* Alignment of method arguments

org.jboss.shrinkwrap.api.container.LibraryContainer
---------------------------------------------------

* addLibraries()

org.jboss.shrinkwrap.api.export.FactoryUtil
-------------------------------------------

* createInstance() with ClassLoader

[jesper.pedersen]

Check the alignment of method parameters in all factories

./api-as/src/main/java/org/jboss/bootstrap/api/as/server/JBossASServerFactory.java
./api-as/src/main/java/org/jboss/bootstrap/api/as/config/JBossASServerConfigFactory.java
./api-embedded/src/main/java/org/jboss/bootstrap/api/embedded/server/JBossASEmbeddedServerFactory.java
./api/src/main/java/org/jboss/bootstrap/api/factory/GenericFactory.java
./api/src/main/java/org/jboss/bootstrap/api/factory/ServerFactory.java
./api/src/main/java/org/jboss/bootstrap/api/factory/ServerConfigFactory.java
./api-mc/src/main/java/org/jboss/bootstrap/api/mc/server/MCServerFactory.java
./api-mc/src/main/java/org/jboss/bootstrap/api/mc/config/MCServerConfigFactory.java

so the order of the parameters are located in the same order

blabla(final ClassLoader cl)
blabla(final ClassLoader cl, final String clz)

[jesper.pedersen]

ShrinkWrap: All set/getContextClassLoader method calls should be wrapped in a doPrivileged()

[alrubinger]

"Jesper Pedersen" wrote:
Overall:
========

* Good idea to add overview.html / package.html

https://jira.jboss.org/jira/browse/JBBOOT-112
https://jira.jboss.org/jira/browse/SHRINKWRAP-58

"Jesper Pedersen" wrote:
* Good idea to run checkstyle
* Good idea to run findbugs (with reportLevel="low")

Will look into these. I'm not convinced of the benefits to CheckStyle.

"Jesper Pedersen" wrote:
Bootstrap:
==========
* api-embedded
* impl-embedded

- Needs to be move to Embedded - as they are a specific use-case of Bootstrap
- Refactor package names

Emanuel agrees, now I do too. @see http://www.jboss.org/index.html?module=bb&op=viewtopic&t=162643. These can be moved as-is to org.jboss.embedded. However for the time being we'll keep the API of Embedded the same as Bootstrap. Later on we'll piece together by composition (Bootstrap for lifecycle, PS for Config, TMPDPL for Deployment).

"Jesper Pedersen" wrote:
* I'm not too crazy about BootstrapMetadata being in the spi/ package
* Also there are constants in spi/ which may not have a relevance for the actual environment

This really hasn't been touched much from the legacy (1.0.0-X) bootstrap impl. It's used in parsing out the bootstrap XML descriptors. Doesn't need to be in API and exposed to users, but needs to be locked down. Hence SPI.

"Jesper Pedersen" wrote:
* I guess it is ok to keep the MC and AS specific modules here - as many projects will use them

I don't know if that's really true either. :) I think api-as/impl-as should definitely move to modules within the AS. If the MC team wants to adopt the MC bootstrap components that'd make sense too.

"Jesper Pedersen" wrote:
* Missing excludes in pom.xml for MC and AS

Which ones? :) It's likely that projects we're consuming should instead be defining optional==true for their transitive deps. But this is an issue across all projects I've seen.

"Jesper Pedersen" wrote:
org.jboss.bootstrap.api.server.Server
-------------------------------------
void registerEventHandler(LifecycleState state, LifecycleEventHandler handler) throws IllegalArgumentException;

arguments should be reversed to follow other registerEventHandler methods

The reason for this is due to the varargs requirement to be last, and this is an overloaded method. Perhaps we should explore alternative method names to make things more clear? That's a Bloch point. ;)

"Jesper Pedersen" wrote:
org.jboss.bootstrap.api.lifecycle.LifecycleState
------------------------------------------------

Are we sure that there won't be any additional server states in the future ?
Or that an implementation will define additional states ?

In those cases an enum won't work - as it can't be extended.

Bootstrap currently supports a finite/immutable set of states. Unlike MC. :)

"Jesper Pedersen" wrote:
Embedded:
=========

* Missing excludes in pom.xml

Same point as above. Or I could declare all deps as optional. But I'd want to know specifically what problems we're reaching to solve.

"Jesper Pedersen" wrote:
org.jboss.bootstrap.api.embedded.server.JBossASEmbeddedServerFactory
--------------------------------------------------------------------

"public static final ..." -> "private static final ..."

... otherwise a

createServer(final ClassLoader cl, final String className)

method is needed...

You're referring to "DEFAULT_SERVER_IMPL_CLASS_NAME". https://jira.jboss.org/jira/browse/JBBOOT-113 and done.

"Jesper Pedersen" wrote:
ShrinkWrap:
===========

- Check ContextClassLoader assumptions

I found as an example JavaArchiveFactory does not provide a way for the user to specify the ClassLoader to be used in creating an instance. We assume the TCCL is sufficient. We'll do a full review on this.

https://jira.jboss.org/jira/browse/SHRINKWRAP-59

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.Path
-----------------------------

* Would a org.jboss.cache.Fqn<E> style be better ?

In any case, we need a factory method to create Paths, otherwise users with only the API on their classpath will get some NCDFE when BasicPath is encountered.

https://jira.jboss.org/jira/browse/SHRINKWRAP-57

I think Path is sufficient for us when contrasted with the broader support offered by FQN.

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.Archive
--------------------------------

T add(Path target, String name, Asset asset) throws IllegalArgumentException;

switch assert and name

https://jira.jboss.org/jira/browse/SHRINKWRAP-60

"Jesper Pedersen" wrote:
* Remove all "String target" -- Path should be used

We've debated this a bunch and came to the conclusion that we need to accept the the String form of "target". The API makes for method chaining and that quickly gets muddled when you need to introduce "new BasicPath("path")" all over.

"Jesper Pedersen" wrote:
T merge(Path path, Archive<?> source) throws IllegalArgumentException;

switch path and source

https://jira.jboss.org/jira/browse/SHRINKWRAP-61

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.spec.FactoryUtil
-----------------------------------------

* createInstance method with ClassLoader

Covered by SHRINKWRAP-59 above.

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.container.ResourceContainer
org.jboss.shrinkwrap.api.container.EnterpriseContainer
org.jboss.shrinkwrap.api.container.WebContainer
org.jboss.shrinkwrap.api.container.ManifestContainer
----------------------------------------------------

* Alignment of method arguments

It's "path, resource, classloader" in all places I've seen.

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.container.LibraryContainer
---------------------------------------------------

* addLibraries()

https://jira.jboss.org/jira/browse/SHRINKWRAP-62

"Jesper Pedersen" wrote:
org.jboss.shrinkwrap.api.export.FactoryUtil
-------------------------------------------

* createInstance() with ClassLoader

Above, again. :)

Thanks for the review.

S,
ALR

[jesper.pedersen]

If there are any constraints on which JAR files that must be on the application classloader - these must be documented too.

[jesper.pedersen]

Will look into these. I'm not convinced of the benefits to CheckStyle.

The checkstyle idea was to find all places that are missing JavaDoc...

It's "path, resource, classloader" in all places I've seen.

I have added a comment to SHRINKWRAP-61