You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by m-czernek <gi...@git.apache.org> on 2018/10/07 12:42:03 UTC
[GitHub] tomcat pull request #125: Provide port offset functionality (BZ-61171)
GitHub user m-czernek opened a pull request:
https://github.com/apache/tomcat/pull/125
Provide port offset functionality (BZ-61171)
This PR resolves [BZ61171](https://bz.apache.org/bugzilla/show_bug.cgi?id=61171) and adds the port offset functionality.
The implementation is deliberately done such that the current behavior is left unchanged, i.e. `getPort()` always returns the port without offset. However, if you set `portOffset` on the `StandardServer` element, all connectors get the offset, and are listening at port+portOffset port. To get port with offset, one specifically has to call `getPortWithOffset()`. This behavior is also signified in the logs, which now show, for example, `"http-nio-8080-offset-100"` rather than `"http-nio-8180"`.
User-facing documentation will follow when and if this PR gets merged.
Thoughts? As always, I'm keen on feedback and request for changes. Alternatively, feel free to change any part of the PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/m-czernek/tomcat bz61171_portOffset
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tomcat/pull/125.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 #125
----
commit a9162b7112719a5cb649af2cc673239db92b5f0e
Author: Marek Czernek <mc...@...>
Date: 2018-02-13T15:19:34Z
Provide port offset functionality
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by markt-asf <gi...@git.apache.org>.
Github user markt-asf commented on the issue:
https://github.com/apache/tomcat/pull/125
Patch has been applied with a few changes:
- portOffset was not cached on the Connector (to align with recent changes to how port is handled)
- handle special case port values where the offset should not be applied
- use getPortWithOffset() everywhere
- expose new attributes via JMX
- take a slightly different approach with logging in case anyone (not that they should) is parsing log messages based on the current format
- added handling for the redirectPort attribute
- fix any IDE / CheckStyle warnings
Despite what might look like a long list of changes, the patch was applied largely as-is. I particularly like the approach to handling the Connectors.
Many thanks. This feature will be available in 9.0.13 onwards.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat pull request #125: Provide port offset functionality (BZ-61171)
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tomcat/pull/125
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by markt-asf <gi...@git.apache.org>.
Github user markt-asf commented on the issue:
https://github.com/apache/tomcat/pull/125
I've skimmed this and it looks good. I'm currently buried in TLS 1.3 stuff but wanted to let you know that the PR had been seen and that I hope to get it into the next 9.0.x release.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by m-czernek <gi...@git.apache.org>.
Github user m-czernek commented on the issue:
https://github.com/apache/tomcat/pull/125
@markt-asf I'll check your review into a lot more detail later, possibly tonight, but at this moment:
- You set `portOffset` on Server
- All connectors are offset
There is no portOffset on individual Connectors. When each connector gets initialized, it starts with `portWithOffset`, basically `port` and `offset` being exposed for legacy reasons.
You suggest to provide `offset` for individual connectors as well? Because for that, we don't have to have a portOffset, we can just change the ports manually :). I think I'm just misunderstanding here; I'll have to check out the `Connector` <-> `Endpoint` relationship.
This is, by the way, in line with Wildfly, where you set one property with 'portOffset' (or you specify it in the upper-most xml element of socket binding group) and all the ports get offset. Another possible, and very simple, solution is also to include in the default configuration some system property, that will be simply added to all ports if the property is set. Or, simply add the property value to all port numbers if the property exists...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by markt-asf <gi...@git.apache.org>.
Github user markt-asf commented on the issue:
https://github.com/apache/tomcat/pull/125
I've now spent some time looking at this more closely.
I like the idea of setting this once on the `Server` and then auto-magically setting this on the `Connector`s.
As I reviewed this, I noticed that we are still setting the port in multiple places. That seems wrong to me as it creates the possibility of having inconsistent settings. I am leaning towards refactoring `port` in the `Connector` so it always delegates to the `Endpoint`. `portOffset` would then be handled the same way.
I am a little concerned about having `portOffset` on the `Server` and the `Connector`. Again, there is scope there for the settings to become inconsistent. However, I don't see any easy way around that.
Finally, I am still mulling over the extent to which the currently used port is exposed as `portWithOffset` and when `port` and `offset` are exposed separately. I'm leaning towards a wider use of `portWithOffset` and separate log messages that provide `portWithOffset`, `port` and `offset`.
My current plan is to refactor port as describe above and then start to integrate this patch. I'm expecting to tweak a few things as I go and I'm still aiming for inclusion in 9.0.13.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by isapir <gi...@git.apache.org>.
Github user isapir commented on the issue:
https://github.com/apache/tomcat/pull/125
> Another possible, and very simple, solution is also to include in the default configuration some system property, that will be simply added to all ports if the property is set. Or, simply add the property value to all port numbers if the property exists...
I think that in general we avoid System properties, but for this feature it might make sense - the reason being that this feature is mostly useful in environments where there are many different instances of Tomcat, and in those cases the deployment is scripted.
For example, one of the organizations that I advise has about 200 instances of Tomcat, all deployed in the same manner, so a script generates all of the CATALINA_BASE directories, and in that case it's much easier to add a system property rather than patch server.xml. In fact, the way that I usually address this is by setting a property and then specifying it server.xml, so that server.xml can be copied with no changes, e.g.
port="${tomcat.port}"
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] tomcat issue #125: Provide port offset functionality (BZ-61171)
Posted by rmaucher <gi...@git.apache.org>.
Github user rmaucher commented on the issue:
https://github.com/apache/tomcat/pull/125
Ok, so I'm guilty for all the system properties in Tomcat ... Sorry. So now, it's not good anymore. System properties are still fine for end users, but only by using ${...} property replacement in server.xml.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org