WIP Xpack spi request #60220

pgomulka · 2020-07-27T14:59:40Z

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS and architecture that we support?
If you are submitting this code for a class then read our policy for that.

jakelandis

Looking good. I like this approach, but think it would be cleaner if we passed a Function<String,Version> from the plugin.

jakelandis · 2020-07-27T16:08:35Z

server/src/main/java/org/elasticsearch/plugins/RestCompatibilityPlugin.java

+import org.elasticsearch.Version;
+
+public interface RestCompatibilityPlugin {
+    Version minimumRestCompatibilityVersion();


nit: I would suggest to just call this restCompatibleVersion including minimum implies there is a maximum and things in-between which there may be in future versions but for not a single version seems right.

jakelandis · 2020-07-27T16:28:25Z

server/src/main/java/org/elasticsearch/plugins/RestCompatibilityPlugin.java

+
+public interface RestCompatibilityPlugin {
+    Version minimumRestCompatibilityVersion();
+}


Instead of a Version method, can this be a Function that accepts a String (the header) and returns Version such that that the function can passed into the RestController and applied as need. It will help to keep the logic encapsulated in the plugin, such that the OSS code only needs to know about the header so it can pass it to the function.

I am actually not convinced this will be a good idea. I know that we want to as much compatible code into xpack plugin, but from Object Oriented model of RestRequest that function really belongs there.
I made an attempt to show you how it would look if we extract this.
The main problem is that we essentially are exposing RestRequest details (although these are still public - hasContent and headers) and pass it over to a function implemented in a xpack plugin.
Take a look and let me know

@pgomulka I submitted a PR to your branch pgomulka#21 with some thoughts on how this can be tweaked to be a bit cleaner.

The changes suggested (in PR):

remove need for hasContent(), and simply look at the existence of Content-Type

pass only the Accept and Content-Type headers through the function

create the function as soon as possible and pass that around

minor naming/organization such that the plugin happens to implement the functional interface, (as opposed to IS the interface)

minor tweaking to the validation rules for the headers (since don't have hasContent())

move all parsing logic to plugin. (some may need to go back in xContent, but for now I think this should work)

I am still +1 to functional over OO, maybe the proposed changes will help to change your mind ? Let me know.

jakelandis · 2020-07-27T16:29:52Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

+
+
+    private boolean isRequestingCompatibility() {
+      /*  String acceptHeader = header(CompatibleConstants.COMPATIBLE_ACCEPT_HEADER);


If we pass around a function we can put this logic in the plugin. Calling the function or not can be determined if the header exists at all or not.

see #60220 (comment)

jakelandis · 2020-07-27T16:34:56Z

server/src/main/java/org/elasticsearch/Version.java

+        CompatibleApiVersionProvider compatibleApiVersionProvider = first.orElseGet(() -> () -> Version.CURRENT);
+        // TODO should we fetch that dynamically when calling minimumRestCompatibilityVersion() ?
+        minimumRestCompatibilityVersion = compatibleApiVersionProvider.minimumRestCompatibilityVersion();
+        System.out.println(minimumRestCompatibilityVersion);


this is nice to have ... but I don't think it is needed since it will always be current major-1. If i understand correctly this also requires an additional interface ?

Can we remove this ?

jakelandis · 2020-07-27T16:36:10Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+    private Version getMinimumRestCompatibilityVersion(List<RestCompatibilityPlugin> restCompatPlugins) {
+        return restCompatPlugins.stream()
+            .map(RestCompatibilityPlugin::minimumRestCompatibilityVersion)
+            .max(Version::compareTo)


Can we through an exception if there are multiple plugins that implement the interface ?

right, that method always return current -1 so older APIs will not be accessible (verison -2 - compat apis present still in for a short period of time after version bump)

jakelandis · 2020-07-27T16:37:13Z

server/src/main/java/org/elasticsearch/plugins/spi/CompatibleApiVersionProvider.java

+
+import org.elasticsearch.Version;
+
+public interface CompatibleApiVersionProvider {


Can this be removed if we remove the println from the Version class ?

jakelandis · 2020-07-27T16:38:14Z

server/src/main/java/org/elasticsearch/rest/RestController.java

        this.headersToCopy = headersToCopy;
        this.usageService = usageService;
+        this.minimumRestCompatibilityVersion = minimumRestCompatibilityVersion;


nit: restCompatibleVersion

jakelandis · 2020-07-27T16:39:10Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

+     * Only a major Versions are supported. Internally we use Versions objects, but only use Version(major,0,0)
+     * @return a version with what a client is compatible with.
+     */
+    public Version getCompatibleApiVersion(Version minimumRestCompatibilityVersion) {


nit: maybeGetCompatibleApiVersion

jakelandis · 2020-07-27T16:42:44Z

...est-request/src/main/java/org/elasticsearch/compat/PreviousVersionCompatibleApiProvider.java

+
+//TODO I don't think we want SPI approach. to load spi with ServiceLoader we need to fetch a plugin with the same classloader
+// that means we can get the minimumRestCompatibilityVersion from the plugin itself
+public class PreviousVersionCompatibleApiProvider implements CompatibleApiVersionProvider {


Can we get rid of this if we remove the println from Version ?

pgomulka · 2020-07-27T15:01:22Z

server/src/main/java/org/elasticsearch/plugins/spi/CompatibleApiVersionProvider.java

+
+import org.elasticsearch.Version;
+
+public interface CompatibleApiVersionProvider {


O I don't think we want SPI approach. to load spi with ServiceLoader we need to fetch a plugin with the same classloader
we can use that plugin directly

Some revisions to the idea of using a function

pgomulka added 2 commits July 27, 2020 15:27

draft

c6aeb38

compatible version from a plugin

e85833e

pgomulka changed the title ~~Xpack spi request~~ WIP Xpack spi request Jul 27, 2020

pgomulka added the WIP label Jul 27, 2020

jakelandis requested changes Jul 27, 2020

View reviewed changes

pgomulka commented Jul 27, 2020

View reviewed changes

pgomulka and others added 10 commits July 28, 2020 14:41

moving compatibility function to a plugin

7f8b84a

remove unused interface

88598dc

suggested changes

c009235

cleanup

dce47ac

more cleanup

3470081

Merge pull request #21 from jakelandis/jake-xpack-spi-request

72d7b13

Some revisions to the idea of using a function

interface instead of function

6b06d7f

testcase for combinations

5ec8038

add hascontent argumetn and pass tests

a45171e

precommit

4ccf457

pgomulka mentioned this pull request Sep 1, 2020

Compatible API header parsing plugin #60516

Closed

pgomulka closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Xpack spi request #60220

WIP Xpack spi request #60220

pgomulka commented Jul 27, 2020

jakelandis left a comment

jakelandis Jul 27, 2020

jakelandis Jul 27, 2020

pgomulka Jul 28, 2020

jakelandis Jul 28, 2020

jakelandis Jul 27, 2020

pgomulka Jul 28, 2020

jakelandis Jul 27, 2020

jakelandis Jul 27, 2020

pgomulka Jul 27, 2020

jakelandis Jul 27, 2020

pgomulka Jul 27, 2020

jakelandis Jul 27, 2020

jakelandis Jul 27, 2020

jakelandis Jul 27, 2020

pgomulka Jul 27, 2020



		private boolean isRequestingCompatibility() {
		/* String acceptHeader = header(CompatibleConstants.COMPATIBLE_ACCEPT_HEADER);


		import org.elasticsearch.Version;

		public interface CompatibleApiVersionProvider {

WIP Xpack spi request #60220

WIP Xpack spi request #60220

Conversation

pgomulka commented Jul 27, 2020

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment