cleanup of the JettyServletEngineAdapters and JettyHttpProxy for Jetty 9.4 and 12 #309
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
…y 9.4 and 12 Signed-off-by: Lachlan Roberts <[email protected]>
|
This is targeted at the |
|
@maigovannon will do the full review. |
| // Don't add the RPC Connector if in HttpConnector mode. | ||
| if (!isHttpConnectorMode) | ||
| { | ||
| rpcConnector = |
There was a problem hiding this comment.
I understand that the rpcConnector won't be needed for HTTP mode but with this change we are no longer initializating this field anymore for both modes. Any chance that it would cause an NPE below for HttpConnector when we actually service the request?
There was a problem hiding this comment.
This field is only ever used in JettyServletEngineAdaptor.serviceRequest, and in the GEN2 runtimes the only way we get to serviceRequest is through the ForwardingHandler in the JettyHttpProxy, and in HttpConnectorMode we never add this handler to go over RPC.
| if (!isHttpConnectorMode) | ||
| { |
There was a problem hiding this comment.
[nit] Suggest we stick to the K&R style since that is the convention in most other places.
| HttpConfiguration httpConfiguration = rpcConnector.getHttpConfiguration(); | ||
| httpConfiguration.setSendDateHeader(false); | ||
| httpConfiguration.setSendServerVersion(false); | ||
| httpConfiguration.setSendXPoweredBy(false); | ||
| if (LEGACY_MODE) { | ||
| httpConfiguration.setRequestCookieCompliance(CookieCompliance.RFC2965); | ||
| httpConfiguration.setResponseCookieCompliance(CookieCompliance.RFC2965); | ||
| httpConfiguration.setUriCompliance(UriCompliance.LEGACY); | ||
| } | ||
|
|
There was a problem hiding this comment.
What are your thoughts about movign all this to the constructor of DelegateConnector itself? I see that it class is only here and if you feel you don't want to give these defaults for all constructors of this instance, should we create static createDefaultDelegateConnector() ?
There was a problem hiding this comment.
What are your thoughts about movign all this to the constructor of DelegateConnector itself?
I wrote the jetty-delegate code to be a generic mechansim not google specific things, which could potentially be applied to other protocols other than the google RPC, so that it could potentially be extracted out into Jetty to be maintained in the future.
The interface with google code is done in the com.google.apphosting.runtime.jetty.delegate.impl package.
So I think adding compliance modes specific for the google usage should be here rather than in the DelegateConnector.
There was a problem hiding this comment.
Would that fix the flakiness seen with 12.0.15 #304 ?
There was a problem hiding this comment.
Don't think so, I compared both SizeLimitHandlers and they were essentially the same other than some improved error handling in the Jetty one.
Opened #310 to track this.
| }; | ||
| server.addConnector(rpcConnector); | ||
|
|
||
| // Don't add the RPC Connector if in HttpConnector mode. |
There was a problem hiding this comment.
Hum. so we did that, what the side effect? 2 connectors and one not used? More memory used?
There was a problem hiding this comment.
Probably, but I don't think the memory saving here is that significant considering the DelegateConnector was never used.
| httpConfiguration.setSendDateHeader(false); | ||
| httpConfiguration.setSendServerVersion(false); | ||
| httpConfiguration.setSendXPoweredBy(false); | ||
| if (LEGACY_MODE) { |
There was a problem hiding this comment.
Just a question there (hard to diff), LEGACY_MODE is still in both rpc and http connector, right?
There was a problem hiding this comment.
Yes but for http connector it is done in com.google.apphosting.runtime.jetty.proxy.JettyHttpProxy#newConnector.
Signed-off-by: Lachlan Roberts <[email protected]>
…tEngineAdaptors PiperOrigin-RevId: 697802010 Change-Id: I1f6149e6fd89a1ab5d1ce75b9ae416b8f0fb4faf
insertHandler()where possible.HttpConnectormode.HttpConfigurationonce.SizeLimitHandlersinto one instead of having separate ones for request and response content.CoreSizeLimitHandlerand use the one provided by Jetty.