-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow registering compatible handlers #64423
Changes from 23 commits
e468fdf
5cfa11e
66224e7
29b27e8
4690362
e7866a9
0330d97
78d4660
bc5de8b
f958cdd
2bda74f
7779083
dd26408
94b4a0a
4385591
dc61731
4a3138d
0b565e5
b35ad2c
b908bfe
950ba7b
310b735
afba70d
385f5a9
836fe0f
8f2ea92
d70a071
87d762f
deff739
4ecbf22
a38ed04
c39d0bb
09704f3
023c8d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -48,6 +48,8 @@ | |||||
*/ | ||||||
public final class XContentBuilder implements Closeable, Flushable { | ||||||
|
||||||
private byte compatibleMajorVersion; | ||||||
|
||||||
/** | ||||||
* Create a new {@link XContentBuilder} using the given {@link XContent} content. | ||||||
* <p> | ||||||
|
@@ -1004,6 +1006,15 @@ public XContentBuilder copyCurrentStructure(XContentParser parser) throws IOExce | |||||
return this; | ||||||
} | ||||||
|
||||||
public XContentBuilder withCompatibleMajorVersion(byte compatibleMajorVersion){ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
this.compatibleMajorVersion = compatibleMajorVersion; | ||||||
return this; | ||||||
} | ||||||
|
||||||
public byte getCompatibleMajorVersion() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add javadocs to this method and the one above? |
||||||
return compatibleMajorVersion; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public void flush() throws IOException { | ||||||
generator.flush(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -259,6 +259,7 @@ private static Version fromStringSlow(String version) { | |
public final byte build; | ||
public final org.apache.lucene.util.Version luceneVersion; | ||
private final String toString; | ||
public final int previousMajorId; | ||
pgomulka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Version(int id, org.apache.lucene.util.Version luceneVersion) { | ||
this.id = id; | ||
|
@@ -268,6 +269,15 @@ private static Version fromStringSlow(String version) { | |
this.build = (byte) (id % 100); | ||
this.luceneVersion = Objects.requireNonNull(luceneVersion); | ||
this.toString = major + "." + minor + "." + revision; | ||
this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major; | ||
} | ||
|
||
public Version previousMajor() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. javadocs please |
||
return Version.fromId(previousMajorId); | ||
} | ||
|
||
public static Version minimumRestCompatibilityVersion() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, should we make this non-static like |
||
return Version.CURRENT.previousMajor(); | ||
} | ||
|
||
public boolean after(Version version) { | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -149,6 +149,7 @@ | |||
import org.elasticsearch.plugins.SystemIndexPlugin; | ||||
import org.elasticsearch.repositories.RepositoriesModule; | ||||
import org.elasticsearch.repositories.RepositoriesService; | ||||
import org.elasticsearch.rest.CompatibleVersion; | ||||
import org.elasticsearch.rest.RestController; | ||||
import org.elasticsearch.script.ScriptContext; | ||||
import org.elasticsearch.script.ScriptEngine; | ||||
|
@@ -539,9 +540,11 @@ protected Node(final Environment initialEnvironment, | |||
repositoriesServiceReference::get).stream()) | ||||
.collect(Collectors.toList()); | ||||
|
||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
ActionModule actionModule = new ActionModule(settings, clusterModule.getIndexNameExpressionResolver(), | ||||
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(), | ||||
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices); | ||||
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices, | ||||
getRestCompatibleFunction()); | ||||
modules.add(actionModule); | ||||
|
||||
final RestController restController = actionModule.getRestController(); | ||||
|
@@ -716,6 +719,15 @@ protected Node(final Environment initialEnvironment, | |||
} | ||||
} | ||||
|
||||
/** | ||||
* @return A function that can be used to determine the requested REST compatible version | ||||
* package scope for testing | ||||
*/ | ||||
CompatibleVersion getRestCompatibleFunction() { | ||||
// TODO PG Until compatible version plugin is implemented, return current version. | ||||
return CompatibleVersion.CURRENT_VERSION; | ||||
} | ||||
|
||||
protected TransportService newTransportService(Settings settings, Transport transport, ThreadPool threadPool, | ||||
TransportInterceptor interceptor, | ||||
Function<BoundTransportAddress, DiscoveryNode> localNodeFactory, | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.rest; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.xcontent.ParsedMediaType; | ||
|
||
/** | ||
* An interface used to specify a function that returns a compatible API version | ||
* Intended to be used in a code base instead of a plugin. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure i follow the second sentence (Intended to be ...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right.. I rephrased this, but maybe it is still to vague. let me know |
||
*/ | ||
@FunctionalInterface | ||
public interface CompatibleVersion { | ||
Version get(@Nullable ParsedMediaType acceptHeader, @Nullable ParsedMediaType contentTypeHeader, boolean hasContent); | ||
|
||
CompatibleVersion CURRENT_VERSION = (acceptHeader, contentTypeHeader, hasContent) -> Version.CURRENT; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,7 +19,7 @@ | |||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
package org.elasticsearch.rest; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import org.elasticsearch.common.Nullable; | ||||||||||||||||||||||||||||||
import org.elasticsearch.Version; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import java.util.HashMap; | ||||||||||||||||||||||||||||||
import java.util.Map; | ||||||||||||||||||||||||||||||
|
@@ -31,23 +31,25 @@ | |||||||||||||||||||||||||||||
final class MethodHandlers { | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private final String path; | ||||||||||||||||||||||||||||||
private final Map<RestRequest.Method, RestHandler> methodHandlers; | ||||||||||||||||||||||||||||||
private final Map<RestRequest.Method, Map<Version, RestHandler>> methodHandlers; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
MethodHandlers(String path, RestHandler handler, RestRequest.Method... methods) { | ||||||||||||||||||||||||||||||
MethodHandlers(String path, RestHandler handler, Version version, RestRequest.Method... methods) { | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you a few unit tests for this class ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also realised that changing the method signature was not necessary. |
||||||||||||||||||||||||||||||
this.path = path; | ||||||||||||||||||||||||||||||
this.methodHandlers = new HashMap<>(methods.length); | ||||||||||||||||||||||||||||||
for (RestRequest.Method method : methods) { | ||||||||||||||||||||||||||||||
methodHandlers.put(method, handler); | ||||||||||||||||||||||||||||||
methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) | ||||||||||||||||||||||||||||||
.put(version, handler); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||
* Add a handler for an additional array of methods. Note that {@code MethodHandlers} | ||||||||||||||||||||||||||||||
* does not allow replacing the handler for an already existing method. | ||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||
MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { | ||||||||||||||||||||||||||||||
MethodHandlers addMethods(RestHandler handler, Version version, RestRequest.Method... methods) { | ||||||||||||||||||||||||||||||
for (RestRequest.Method method : methods) { | ||||||||||||||||||||||||||||||
RestHandler existing = methodHandlers.putIfAbsent(method, handler); | ||||||||||||||||||||||||||||||
RestHandler existing = methodHandlers.computeIfAbsent(method, k -> new HashMap<>()) | ||||||||||||||||||||||||||||||
.putIfAbsent(version, handler); | ||||||||||||||||||||||||||||||
if (existing != null) { | ||||||||||||||||||||||||||||||
throw new IllegalArgumentException("Cannot replace existing handler for [" + path + "] for method: " + method); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
@@ -56,11 +58,17 @@ MethodHandlers addMethods(RestHandler handler, RestRequest.Method... methods) { | |||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||
* Returns the handler for the given method or {@code null} if none exists. | ||||||||||||||||||||||||||||||
* Returns the handler for the given method and version. | ||||||||||||||||||||||||||||||
* If a handler for given version do not exist, a handler for Version.CURRENT will be returned. | ||||||||||||||||||||||||||||||
* or {@code null} if none exists. | ||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||
@Nullable | ||||||||||||||||||||||||||||||
RestHandler getHandler(RestRequest.Method method) { | ||||||||||||||||||||||||||||||
return methodHandlers.get(method); | ||||||||||||||||||||||||||||||
RestHandler getHandler(RestRequest.Method method, Version version) { | ||||||||||||||||||||||||||||||
Map<Version, RestHandler> versionToHandlers = methodHandlers.get(method); | ||||||||||||||||||||||||||||||
if (versionToHandlers == null) { | ||||||||||||||||||||||||||||||
return null; //method not found | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
final RestHandler handler = versionToHandlers.get(version); | ||||||||||||||||||||||||||||||
return handler != null || version.equals(Version.CURRENT) ? handler : versionToHandlers.get(Version.CURRENT); | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really can't remember the use cases where we needed this. (returning a v8 handler when a handler was not present in v7) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the reason for this is @Mpdreamz's comment #60516 (comment) where clients plan to send compatible with to all APIs. If this is the case, can we please document this in code/method so we do not lose the information There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to think about this waaay to long :) and wrote a unit test to help me understand
Can you add a similar comment inline here, and add that test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return can be simplified to:
with a test:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be fair, I am not super convinced about returning the CURRENT. @jakelandis I am not sure I understand this:
from high level point of view, there is no knowledge if there is a compatible endpoint or not. If the handler's code did not change, user has no idea that a response came from v7 code, or if the response is THe same for when an endpoint do not exist. Returning 404 from v8 when for not existing in v7 endpoint is 100% compatible in shape and behaviour @Mpdreamz any views on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should return new APIs with compatible requested. Below is a table with my thought for how behaviour should look. The rationale is that in a minor you can add a new API passively, therefor new APIs are compatible (as opposed to non-compatible/breaking). Further, one motivation to add a new API is to replace an old one. With a client sending compatibility all the time, they should be able to fix the compatibility warnings and migrate to the new API. This allows them to do just that.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
great explanation. thank you. I think this was the initial reason we went for that behaviour on a feature branch. I will add a comment about this at least in a test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since clients generate methods for known API's the only way to talk to an API that exists in I think the new in v8
The first two seem straight forward the last one not so much It could return a
Returning with Returning with That leaves returning a 404. cc @elastic/es-clients we have not discussed if the the transport used in the client should default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think with the current approach where we version compatible API |
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
import org.apache.logging.log4j.Logger; | ||
import org.apache.logging.log4j.message.ParameterizedMessage; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.client.node.NodeClient; | ||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.Strings; | ||
|
@@ -90,11 +91,14 @@ public class RestController implements HttpServerTransport.Dispatcher { | |
/** Rest headers that are copied to internal requests made during a rest request. */ | ||
private final Set<RestHeaderDefinition> headersToCopy; | ||
private final UsageService usageService; | ||
private CompatibleVersion compatibleVersion; | ||
|
||
public RestController(Set<RestHeaderDefinition> headersToCopy, UnaryOperator<RestHandler> handlerWrapper, | ||
NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService) { | ||
NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService, | ||
CompatibleVersion compatibleVersion) { | ||
this.headersToCopy = headersToCopy; | ||
this.usageService = usageService; | ||
this.compatibleVersion = compatibleVersion; | ||
if (handlerWrapper == null) { | ||
handlerWrapper = h -> h; // passthrough if no wrapper set | ||
} | ||
|
@@ -168,8 +172,12 @@ protected void registerHandler(RestRequest.Method method, String path, RestHandl | |
} | ||
|
||
private void registerHandlerNoWrap(RestRequest.Method method, String path, RestHandler maybeWrappedHandler) { | ||
handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), | ||
(mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)); | ||
final Version version = maybeWrappedHandler.compatibleWithVersion(); | ||
assert Version.minimumRestCompatibilityVersion() == version || Version.CURRENT == version | ||
: "REST API compatibility is only supported for version " + Version.minimumRestCompatibilityVersion().major; | ||
|
||
handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, version, method), | ||
(mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, version, method)); | ||
} | ||
|
||
/** | ||
|
@@ -242,7 +250,11 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl | |
inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength); | ||
} | ||
// iff we could reserve bytes for the request we need to send the response also over this channel | ||
responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength); | ||
// Using a version from a handler because if no handler was found for requested version, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure i understand the purpose of this comment here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my intention was to indicate that even if you requested an |
||
// we would return a handler for CURRENT. Therefore no compatible logic in serialisation (toXContent) should be applied | ||
// see MethodHandlers#getHandler | ||
responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, | ||
handler.compatibleWithVersion()); | ||
// TODO: Count requests double in the circuit breaker if they need copying? | ||
if (handler.allowsUnsafeBuffers() == false) { | ||
request.ensureSafeBuffers(); | ||
|
@@ -318,6 +330,9 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel | |
final String rawPath = request.rawPath(); | ||
final String uri = request.uri(); | ||
final RestRequest.Method requestMethod; | ||
|
||
Version compatibleVersion = this.compatibleVersion. | ||
get(request.getParsedAccept(), request.getParsedContentType(), request.hasContent()); | ||
try { | ||
// Resolves the HTTP method and fails if the method is invalid | ||
requestMethod = request.method(); | ||
|
@@ -329,7 +344,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel | |
if (handlers == null) { | ||
handler = null; | ||
} else { | ||
handler = handlers.getHandler(requestMethod); | ||
handler = handlers.getHandler(requestMethod, compatibleVersion); | ||
} | ||
if (handler == null) { | ||
if (handleNoHandlerFound(rawPath, requestMethod, uri, channel)) { | ||
|
@@ -454,33 +469,40 @@ private static final class ResourceHandlingHttpChannel implements RestChannel { | |
private final RestChannel delegate; | ||
private final CircuitBreakerService circuitBreakerService; | ||
private final int contentLength; | ||
private final Version compatibleVersion; | ||
private final AtomicBoolean closed = new AtomicBoolean(); | ||
|
||
ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength) { | ||
ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength, | ||
Version compatibleVersion) { | ||
this.delegate = delegate; | ||
this.circuitBreakerService = circuitBreakerService; | ||
this.contentLength = contentLength; | ||
this.compatibleVersion = compatibleVersion; | ||
} | ||
|
||
@Override | ||
public XContentBuilder newBuilder() throws IOException { | ||
return delegate.newBuilder(); | ||
return delegate.newBuilder() | ||
.withCompatibleMajorVersion(compatibleVersion.major); | ||
} | ||
|
||
@Override | ||
public XContentBuilder newErrorBuilder() throws IOException { | ||
return delegate.newErrorBuilder(); | ||
return delegate.newErrorBuilder() | ||
.withCompatibleMajorVersion(compatibleVersion.major); | ||
} | ||
|
||
@Override | ||
public XContentBuilder newBuilder(@Nullable XContentType xContentType, boolean useFiltering) throws IOException { | ||
return delegate.newBuilder(xContentType, useFiltering); | ||
return delegate.newBuilder(xContentType, useFiltering) | ||
.withCompatibleMajorVersion(compatibleVersion.major); | ||
} | ||
|
||
@Override | ||
public XContentBuilder newBuilder(XContentType xContentType, XContentType responseContentType, boolean useFiltering) | ||
throws IOException { | ||
return delegate.newBuilder(xContentType, responseContentType, useFiltering); | ||
return delegate.newBuilder(xContentType, responseContentType, useFiltering) | ||
.withCompatibleMajorVersion(compatibleVersion.major); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I wonder if we should make this a
SetOnce
or add validation that if it is not some sentinel value that it cannot be changed again? I'm not sure that I like it being mutable with a builder pattern and allowing it to be set multiple timesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with a regular builder pattern I would be ok with the field being mutable, as it is normally used within some narrow code scope.
With XContentBuilder we often pass it around so makes sense to protect against some accidental changes.
I feel like we should assert about this in testing only though.
I added
assert