-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Extra testing and some cleanups for filtering on field caps #85068
Extra testing and some cleanups for filtering on field caps #85068
Conversation
Hi @romseygeek, I've created a changelog YAML for you. |
So the test is failing because the rest client picks a random host to hit with its request, and we in fact need it to pick the upgraded host so that the new parameters are recognised by the co-ordinating node. |
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.
I left a few comments, thanks for working on this.
Do we still need to expand the serialization unit tests or was that covered in the meantime? Do you plan on opening another PR for the docs improvements?
|
||
@SuppressWarnings("unchecked") | ||
private RestClient getUpgradedNodeClient() throws IOException { | ||
for (HttpHost host : getClusterHosts()) { |
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.
you can plug in a NodeSelector to the RestClient to have it only point to nodes with specific characteristics. Version can be a factor.
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.
Frustratingly that doesn't help here, because the RestClientBuilder doesn't know the version of its Nodes up-front, and so we can't get the version in the NodeSelector.
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.
ok for my understanding, maybe the problem is that the existing client does not point to the upgraded nodes? I guess it depends on how it is initialized. The point of node selector is to influence node selection when making each request, but then all nodes need to be added to the client rotation. I see that the client gets the nodes from tests.rest.cluster set on build.gradle. Is there a chance that not all nodes are included?
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.
All the nodes are included; the problem is that the RestClientBuilder.builder(HttpHost hosts...)
that gets called here then builds its Nodes using plain Node::new
, which doesn't set any Node metadata (including the version).
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.
got it. Only the sniffer sets these automatically as it has knowledge about the metadata, but you can set metadata up yourself by using the builder(Node ... nodes) method. That makes this change not so useful and along the lines of what you already have though. Have a look if it makes sense to make the change or not?
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/FieldCapsIT.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/fieldcaps/ResponseRewriter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java
Show resolved
Hide resolved
Serialization should be covered by the rolling upgrade IT test. I've expanded on the docs a little in the latest update. |
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.
Can we add setting and handling the additional fields in FieldCapabilitiesRequestTests as well as FieldCapabilitiesNodeRequestTests? LGTM besides that, no need for another round.
Thanks for the reviews @javanna! |
isRuntimeField
from FieldTypeLookupRelates to #83636