You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by mikezaccardo <gi...@git.apache.org> on 2015/05/08 17:01:09 UTC
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
GitHub user mikezaccardo opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/633
Add Tomcat 8 Support
A new `brooklyn.entity.webapp.tomcat8` package was created because new `server.xml` and `web.xml` files were required. Those files must be named as such, so they needed to be placed in a separate package since `server.xml` and `web.xml` already exist in the `brooklyn.entity.webapp.tomcat` pacakge (for Tomcat 7).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikezaccardo/incubator-brooklyn feature/tomcat8-support
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/633.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #633
----
commit dda19f2660b16e26666d6e98f245b22f241a1121
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-05-07T16:24:56Z
Add Tomcat8 classes
commit 0931af544a696c8d1068c00359d072cab52d2b85
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-05-07T16:25:21Z
Add Tomcat8 server.xml and web.xml
commit 0b9dbf776bf1a2ec6aa13b6706357205d760c1d4
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-05-07T16:25:41Z
Remove unused logger and imports
commit 47c59dfa9027ada4e3d43b5c574e057d3cea4978
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-05-07T20:49:35Z
Add Tomcat8 tests
commit 4e554d6d83b82b0ddd5df4c78439aadcd4b5bed4
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-05-07T20:55:46Z
Update comment class reference
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-101417532
I will investigate this `CassandraNode` pattern if the same-package-xml-rename does not work, thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-107375902
@mikezaccardo Super. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-107648155
:tada:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-105841381
@mikezaccardo Thanks again. A few more points:
* We should include empty `Tomcat7Driver` and `Tomcat7SshDriver` classes that are annotated `@Deprecated` and extend the new sans-7 classes to give leeway to anyone who has subclassed the old versions.
* I've run classes matching `Tomcat8*IntegrationTest`. All passed but for `Tomcat8ServerSimpleIntegrationTest.detectFailureIfTomcatCantBindToPort`, which failed twice after throwing `java.lang.IllegalStateException: Timeout waiting for SERVICE_UP from Tomcat8ServerImpl{id=HH3Gpo2G}`
* Lastly, it's worth squashing your commits into one before we merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-106041730
Thanks @sjcorbett -- I've added the empty classes, added a comment about the integration test failure (on Tomcat 7 and 8), created a JIRA ticket, and squashed the commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-101589464
@mikezaccardo Happy to collaborate or test things out for you if deployments are failing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/633
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-101413632
Thanks for looking this over @sjcorbett.
Yes, the removal of `JasperListener` from `server.xml` is the main change.
I agree about the dubious creation of a `tomcat8` package. In fact, I originally attempted to keep them in the same package and made the exact changes to `Tomcat8Server` that you described, with similarly named new xml files, but deployments failed. Ultimately I made the new package and kept the xml files with the same name, which worked. I will revisit keeping it the same package because both of our instincts are the same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-101816122
@sjcorbett I've followed your advice about `tomcat` package consolidation; there is no longer a separate `tomcat8` package. Thanks!
I figured out the reason why my `tomcat8-server.xml` and `tomcat8-web.xml` attempt originally failed: I changed the `String` values of the xml files in lines 77 and 78 of `TomcatSshDriver.java` (renamed from `Tomcat7SshDriver.java`) to their new names which is incorrect.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-100889457
Alternatively we could follow the same pattern as `CassandraNode` and have logic in the TomcatServer entity itself to distinguish between the files to use for versions 7 and 8.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Add Tomcat 8 Support
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/633#issuecomment-100871464
@mikezaccardo Thanks for this. What are the differences in the entities between Tomcat 7 and 8? From a scan, there's not much - the new server.xml modifies the AccessLogValve prefix and removes this line:
```xml
<Listener className="org.apache.catalina.core.JasperListener" />
```
The different web.xml file changes the file's schemaLocation and version.
I don't think this warrants a separate package for v8.
I wonder if a simpler approach could maintain the package structure and reuse the existing classes:
* Move server.xml and web.xml to ```.tomcat``` and rename them to something like ~ tomcat8-server.xml and tomcat8-web.xml.
* Add a `Tomcat8Server` interface that extends `TomcatServer` and redefines `SERVER_XML_RESOURCE`, `WEB_XML_RESOURCE` and the appropriate download/version keys. e.g.:
```java
@SetFromFlag("server.xml")
ConfigKey<String> SERVER_XML_RESOURCE = ConfigKeys.newConfigKeyWithDefault(
TomcatServer.SERVER_XML_RESOURCE, "tomat8-server.xml");
```
The existing driver will handle naming the file correctly when deploying entities.
* Add a `Tomcat8ServerImpl` class that extends `TomcatServerImpl` and implements `Tomcat8Server`. It doesn't need to override any existing methods.
Having compared the v7 files vs. the v8 files - but not tested! - I think this would work and saves the duplication.
Any thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---