Skip to content
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

Introduce client feature tracking #31020

Merged
merged 11 commits into from Jun 1, 2018
Merged

Introduce client feature tracking #31020

merged 11 commits into from Jun 1, 2018

Conversation

@jasontedor
Copy link
Member

jasontedor commented Jun 1, 2018

This commit introduces the ability for a client to communicate to the server features that it can support and for these features to be used in influencing the decisions that the server makes when communicating with the client. To this end we carry the features from the client to the underlying stream as we carry the version of the client today. This enables us to enhance the logic where we make protocol decisions on the basis of the version on the stream to also make protocol decisions on the basis of the features on the stream. With such functionality, the client can communicate to the server if it is a transport client, or if it has, for example, X-Pack installed. This enables us to support rolling upgrades from the OSS distribution to the default distribution without breaking client connectivity as we can now elect to serialize customs in the cluster state depending on whether or not the client reports to us using the feature capabilities that it can under these customs. This means that we would avoid sending a client pieces of the cluster state that it can not understand. However, we want to take care and always send the full cluster state during node-to-node communication as otherwise we would end up with different understanding of what is in the cluster state across nodes depending on which features they reported to have. This is why when deciding whether or not to write out a custom we always send the custom if the client is not a transport client and otherwise do not send the custom if the client is transport client that does not report to have the feature required by the custom.

Closes #30731

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Jun 1, 2018

@jasontedor jasontedor requested review from bleskes and ywelsch Jun 1, 2018
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Jun 1, 2018

This commit introduces the ability for a client to communicate to the
server features that it can support and for these features to be used in
influencing the decisions that the server makes when communicating with
the client. To this end we carry the features from the client to the
underlying stream as we carry the version of the client today. This
enables us to enhance the logic where we make protocol decisions on the
basis of the version on the stream to also make protocol decisions on
the basis of the features on the stream. With such functionality, the
client can communicate to the server if it is a transport client, or if
it has, for example, X-Pack installed. This enables us to support
rolling upgrades from the OSS distribution to the default distribution
without breaking client connectivity as we can now elect to serialize
customs in the cluster state depending on whether or not the client
reports to us using the feature capabilities that it can under these
customs. This means that we would avoid sending a client pieces of the
cluster state that it can not understand. However, we want to take care
and always send the full cluster state during node-to-node communication
as otherwise we would end up with different understanding of what is in
the cluster state across nodes depending on which features they reported
to have. This is why when deciding whether or not to write out a custom
we always send the custom if the client is not a transport client and
otherwise do not send the custom if the client is transport client that
does not report to have the feature required by the custom.

Co-authored-by: Yannick Welsch <yannick@welsch.lu>
@jasontedor jasontedor force-pushed the jasontedor:has_xpack branch to e1fbfaf Jun 1, 2018
Copy link
Contributor

ywelsch left a comment

I've left some comments. I'm also wondering if there's an assertion we could add to enforce that x-pack customs are properly marked as such.

@@ -189,7 +190,7 @@ public long getNumberOfTasksOnNode(String nodeId, String taskName) {

@Override
public Version getMinimalSupportedVersion() {
return Version.V_5_4_0;
return Version.V_6_3_0;

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

We cannot change this. This would mean that a mixed 6.2 x-pack / 6.3 x-pack cluster might drop its persistent tasks on the floor.
Instead I suggest to add another method to custom that says something like featureLessSince which returns an optional version (default is Optional.empty()).
We can then override this method for PersistentTasksCustomMetaData to return Version.V_6_3_0.
Finally we'll make shouldSerializeCustom aware of this new featureLessSince method.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

Discussed with @bleskes. Only revert this to return Version.V_5_4_0;, the rest should be covered by other PRs

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed a18f166.

@@ -189,6 +191,10 @@
private static final long NINETY_PER_HEAP_SIZE = (long) (JvmInfo.jvmInfo().getMem().getHeapMax().getBytes() * 0.9);
private static final BytesReference EMPTY_BYTES_REFERENCE = new BytesArray(new byte[0]);

public static final String FEATURE_PREFIX = "client.features";

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

why did you chose the "client" prefix? maybe use "transport.features" instead?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 1f66829.

throw new IllegalArgumentException("feature settings must have default [true] value");
}
});
this.features = new TreeSet<>(defaultFeatures.names()).toArray(new String[defaultFeatures.names().size()]);

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

can you add a comment that the goal of the TreeSet here is to bring the features into consistent order?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 6d529fe.

assertThat(settings.keySet(), hasItem("transport_client"));
assertThat(settings.get("transport_client"), equalTo("true"));
final ThreadContext threadContext = client.threadPool().getThreadContext();
assertEquals("true", threadContext.getHeader("test"));

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

what is this line testing? not relevant to this test method?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

Sorry, I meant to remove that line and the line above it from this test as that functionality is covered in testDefaultHeader (see below). I pushed 82ed4d4.

case FIELD_NAME:
currentFieldName = parser.currentName();
break;
case VALUE_BOOLEAN:

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

I think this method does not need to be implemented for this test. It is buggy anyhow, I think, as this should be VALUE_NUMBER

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 66f2ea9.

public static class NodePlugin extends CustomPlugin {

@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
static final Optional<String> NODE_PLUGIN_FEATURE = Optional.of("node");

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jun 1, 2018

Contributor

looks like this is only used in the method below it, maybe remove the field declaration

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 25a861c.

Copy link
Member

bleskes left a comment

This looks great. I left one important comment about serializing persistent tasks. The rest are nits.

@@ -104,6 +104,8 @@ else if (readableBytes >= TcpHeader.HEADER_SIZE) {
try (ThreadContext context = new ThreadContext(Settings.EMPTY)) {
context.readHeaders(in);
}
// now we decode the features
in.readStringArray();

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

shouldn't we have a version protection here?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

Good catch. I pushed 0b39ce9.

@@ -113,6 +116,14 @@ public void setVersion(Version version) {
this.version = version;
}

public boolean hasFeature(final String feature) {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

can we have a java docs with some explanation of what the features are (or a link to where it's explained).

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 26071ff.

}

public void setFeatures(final Set<String> features) {
this.features = Collections.unmodifiableSet(new HashSet<>(features));

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

assert it's currently empty?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I wrapped this into 26071ff.

@@ -189,7 +190,7 @@ public long getNumberOfTasksOnNode(String nodeId, String taskName) {

@Override
public Version getMinimalSupportedVersion() {
return Version.V_5_4_0;
return Version.V_6_3_0;

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

I'm not sure we can change this - 6.2 with xpack will have problems.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed a18f166.


@Override
public Optional<String> getRequiredFeature() {
return Optional.of("node-and-transport-client");

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

shall we sometime return non existing value here? (or have yet another type)

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I am not sure what you're going for here; the required feature do not have to line up with the custom type name, they just happen to do that in this test but they are orthogonal. Would it help if I used different names for the required feature and the custom type to make this clear?

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

I don't think we currently test a custom that returns no required features in this test. Since this custom is expected to be returned all the time which is the same behavior as having no required features, I wonder if we sometime want to return an empty Optional. My bad for call it "non existing" - I can see how that may be read differently.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 546fe41.

final BytesStreamOutput out = new BytesStreamOutput();
final Version beforeVersion =
randomVersionBetween(random(), VersionUtils.getFirstVersion(), VersionUtils.getPreviousVersion(version));
out.setVersion(beforeVersion);

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 1, 2018

Member

randomly add the required feature and it shouldn't matter?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jun 1, 2018

Author Member

I pushed 8be176c.

jasontedor added 10 commits Jun 1, 2018
@jasontedor

This comment has been minimized.

Copy link
Member Author

jasontedor commented Jun 1, 2018

@bleskes @ywelsch I have responded to your feedback.

@jasontedor

This comment has been minimized.

Copy link
Member Author

jasontedor commented Jun 1, 2018

I'm also wondering if there's an assertion we could add to enforce that x-pack customs are properly marked as such.

I think we can explore this in a follow-up?

@bleskes
bleskes approved these changes Jun 1, 2018
Copy link
Member

bleskes left a comment

LGTM. Good work.

@ywelsch
ywelsch approved these changes Jun 1, 2018
Copy link
Contributor

ywelsch left a comment

LGTM. Thanks!

@jasontedor jasontedor merged commit 4522b57 into elastic:master Jun 1, 2018
3 checks passed
3 checks passed
CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@jasontedor

This comment has been minimized.

Copy link
Member Author

jasontedor commented Jun 1, 2018

I will backport this later today.

jasontedor added a commit that referenced this pull request Jun 1, 2018
This commit introduces the ability for a client to communicate to the
server features that it can support and for these features to be used in
influencing the decisions that the server makes when communicating with
the client. To this end we carry the features from the client to the
underlying stream as we carry the version of the client today. This
enables us to enhance the logic where we make protocol decisions on the
basis of the version on the stream to also make protocol decisions on
the basis of the features on the stream. With such functionality, the
client can communicate to the server if it is a transport client, or if
it has, for example, X-Pack installed. This enables us to support
rolling upgrades from the OSS distribution to the default distribution
without breaking client connectivity as we can now elect to serialize
customs in the cluster state depending on whether or not the client
reports to us using the feature capabilities that it can under these
customs. This means that we would avoid sending a client pieces of the
cluster state that it can not understand. However, we want to take care
and always send the full cluster state during node-to-node communication
as otherwise we would end up with different understanding of what is in
the cluster state across nodes depending on which features they reported
to have. This is why when deciding whether or not to write out a custom
we always send the custom if the client is not a transport client and
otherwise do not send the custom if the client is transport client that
does not report to have the feature required by the custom.

Co-authored-by: Yannick Welsch <yannick@welsch.lu>
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 1, 2018
This commit introduces the ability for a client to communicate to the
server features that it can support and for these features to be used in
influencing the decisions that the server makes when communicating with
the client. To this end we carry the features from the client to the
underlying stream as we carry the version of the client today. This
enables us to enhance the logic where we make protocol decisions on the
basis of the version on the stream to also make protocol decisions on
the basis of the features on the stream. With such functionality, the
client can communicate to the server if it is a transport client, or if
it has, for example, X-Pack installed. This enables us to support
rolling upgrades from the OSS distribution to the default distribution
without breaking client connectivity as we can now elect to serialize
customs in the cluster state depending on whether or not the client
reports to us using the feature capabilities that it can under these
customs. This means that we would avoid sending a client pieces of the
cluster state that it can not understand. However, we want to take care
and always send the full cluster state during node-to-node communication
as otherwise we would end up with different understanding of what is in
the cluster state across nodes depending on which features they reported
to have. This is why when deciding whether or not to write out a custom
we always send the custom if the client is not a transport client and
otherwise do not send the custom if the client is transport client that
does not report to have the feature required by the custom.

Co-authored-by: Yannick Welsch <yannick@welsch.lu>
@jasontedor

This comment has been minimized.

Copy link
Member Author

jasontedor commented Jun 1, 2018

This change is integrated to 6.3 and 6.x and the BWC versions have been adjusted accordingly.

@jasontedor jasontedor deleted the jasontedor:has_xpack branch Jun 1, 2018
dnhatn added a commit that referenced this pull request Jun 2, 2018
* 6.x:
  Adjust BWC version on client features
  Introduce client feature tracking (#31020)
  [DOCS] Make geoshape docs less memory hungry (#31014)
  Fix handling of percent-encoded spaces in Windows batch files (#31034)
  [Docs] Fix a typo in Create Index naming limitation (#30891)
  REST high-level client: add delete ingest pipeline API (#30865)
  Ensure that index_prefixes settings cannot be changed (#30967)
  REST high-level client: add get ingest pipeline API (#30847)
  Cross Cluster Search: preserve remote status code (#30976)
  High-level client: list tasks failure to not lose nodeId (#31001)
  Refactor Sniffer and make it testable (#29638)
  [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026)
  Add an option to split keyword field on whitespace at query time (#30691)
  Allow rollup job creation only if cluster is x-pack ready (#30963)
  Fix interoperability with < 6.3 transport clients (#30971)
  [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960)
  [DOCS] Fixes links (#31011)
  Watcher: Give test a little more time
bleskes added a commit that referenced this pull request Jun 2, 2018
We now serialize a feature array, which takes an extra byte when empty.
dnhatn added a commit that referenced this pull request Jun 2, 2018
* master:
  Avoid randomization bug in FeatureAwareTests
  Adjust BWC version on client features
  Add TRACE, CONNECT, and PATCH http methods (#31035)
  Adjust BWC version on client features
  [DOCS] Make geoshape docs less memory hungry (#31014)
  Fix handling of percent-encoded spaces in Windows batch files (#31034)
  [Docs] Fix a typo in Create Index naming limitation (#30891)
  Introduce client feature tracking (#31020)
  Ensure that index_prefixes settings cannot be changed (#30967)
  REST high-level client: add delete ingest pipeline API (#30865)
  [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026)
  Allow rollup job creation only if cluster is x-pack ready (#30963)
  Fix interoperability with < 6.3 transport clients (#30971)
  Add an option to split keyword field on whitespace at query time (#30691)
  [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960)
  REST high-level client: add get ingest pipeline API (#30847)
  Cross Cluster Search: preserve remote status code (#30976)
  High-level client: list tasks failure to not lose nodeId (#31001)
  [DOCS] Fixes links (#31011)
  Watcher: Give test a little more time
  Reuse expiration date of trial licenses (#30950)
  Remove unused query methods from MappedFieldType. (#30987)
  Transport client: Don't validate node in handshake (#30737)
  [DOCS] Clarify not all PKCS12 usable as truststores (#30750)
  HLRest: Allow caller to set per request options (#30490)
  Remove version read/write logic in Verify Response (#30879)
  [DOCS] Update readme for testing x-pack code snippets (#30696)
  Ensure intended key is selected in SamlAuthenticatorTests (#30993)
  Core: Remove RequestBuilder from Action (#30966)
dnhatn added a commit that referenced this pull request Jun 2, 2018
* master:
  Adapt transport tests for the extra byte introduced in #31020
bleskes added a commit that referenced this pull request Jun 3, 2018
With #31020 we introduced the ability for transport clients to indicate what features they support
in order to make sure we don't serialize object to them they don't support. This PR adapts the
serialization logic of persistent tasks to be aware of those features and not serialize tasks that
aren't supported. 

Also, a version check is added for the future where we may add new tasks implementations and
need to be able to indicate they shouldn't be serialized both to nodes and clients.

As the implementation relies on the interface of `PersistentTaskParams`, these are no longer
optional. That's acceptable as all current implementation have them and we plan to make
`PersistentTaskParams` more central in the future.

Relates to #30731
bleskes added a commit that referenced this pull request Jun 4, 2018
With #31020 we introduced the ability for transport clients to indicate what features they support
in order to make sure we don't serialize object to them they don't support. This PR adapts the
serialization logic of persistent tasks to be aware of those features and not serialize tasks that
aren't supported.

Also, a version check is added for the future where we may add new tasks implementations and
need to be able to indicate they shouldn't be serialized both to nodes and clients.

As the implementation relies on the interface of `PersistentTaskParams`, these are no longer
optional. That's acceptable as all current implementation have them and we plan to make
`PersistentTaskParams` more central in the future.

Relates to #30731
bleskes added a commit that referenced this pull request Jun 4, 2018
With #31020 we introduced the ability for transport clients to indicate what features they support
in order to make sure we don't serialize object to them they don't support. This PR adapts the
serialization logic of persistent tasks to be aware of those features and not serialize tasks that
aren't supported.

Also, a version check is added for the future where we may add new tasks implementations and
need to be able to indicate they shouldn't be serialized both to nodes and clients.

As the implementation relies on the interface of `PersistentTaskParams`, these are no longer
optional. That's acceptable as all current implementation have them and we plan to make
`PersistentTaskParams` more central in the future.

Relates to #30731
@jpountz jpountz added the >feature label Jun 13, 2018
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.