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.

Sign up for GitHub

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

Jump to bottom

REST Client: NodeSelector for node attributes #31296

Merged
merged 7 commits into from
Jun 15, 2018
Merged

REST Client: NodeSelector for node attributes #31296

merged 7 commits into from
Jun 15, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jun 13, 2018

Add a NodeSelector so that users can filter the nodes that receive
requests based on node attributes.

I believe we'll need this to backport #30523 and we want it anyway.

I also added a bash script to help with rebuilding the sniffer parsing
test documents.

@nik9000
REST Client: NodeSelector for node attributes
e384a0a 
Add a `NodeSelector` so that users can filter the nodes that receive
requests based on node attributes.

I believe we'll need this to backport elastic#30523 and we want it anyway.

I also added a bash script to help with rebuilding the sniffer parsing
test documents.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@nik9000
Copy link
Member Author

nik9000 commented Jun 13, 2018

I'm going to push some docs for this in a moment.

@nik9000
Copy link
Member Author

nik9000 commented Jun 13, 2018

Replaces #31276.

@nik9000 nik9000 requested a review from javanna June 13, 2018 13:25
javanna
javanna approved these changes Jun 13, 2018
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few nits, LGTM though

client/rest/src/test/java/org/elasticsearch/client/HasAttributeNodeSelectorTests.java Outdated
assertEquals(expected, nodes);
}

private Node dummyNode(Map<String, List<String>> attributes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchNodesSniffer.java Outdated
/*
* Multi-valued attributes come with key = `real_key.index` and we
* unflip them after reading them because we can't rely on the that
* they arive in the result object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! We can't rely on the order that they arrive. Proxies could jumble them up and we should still work. 2.x looks like it jumbles them up for fun.

client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchNodesSniffer.java
Map<String, List<String>> realAttributes = new HashMap<>(protoAttributes.size());
List<String> keys = new ArrayList<>(protoAttributes.keySet());
for (String key : keys) {
if (key.endsWith(".0")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh man we don't get the array but rather each item this way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes....

client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchNodesSniffer.java Outdated
/**
* Returns {@code defaultValue} if the attribute didn't come back,
* {@code true} or {@code false} if it did come back as
* either of those, or throws and IOException if the attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throws an

client/sniffer/src/test/resources/create.bash Outdated
@@ -0,0 +1,107 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! maybe rename the file to create_nodes_info_output ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java Outdated
key = parser.currentName();
} else {
NodeSelector newSelector = new HasAttributeNodeSelector(key, parser.text());
parser.skipChildren();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to skip children here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me dig on these. I think they are nice if you pass in weird stuff. Because we don't check the type and just call text. I'll see.

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java Outdated

private static NodeSelector parseVersionSelector(XContentParser parser) throws IOException {
Version[] range = SkipSection.parseVersionRange(parser.text());
parser.skipChildren();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to skip children here?

@nik9000 nik9000 merged commit 856936c into elastic:master Jun 15, 2018
dnhatn added a commit that referenced this pull request Jun 15, 2018
@dnhatn
Merge branch 'master' into ccr
a8abf0f 
* master:
  Upgrade to Lucene-7.4.0-snapshot-518d303506 (#31360)
  Rankeval: Fold template test project into main module (#31203)
  Add QA project and fixture based test for discovery-ec2 plugin (#31107)
  [Docs] Remove reference to repository-s3 plugin creating an S3 bucket (#31359)
  REST Client: NodeSelector for node attributes (#31296)
  LLClient: Fix assertion on windows
  Add details section for dcg ranking metric (#31177)
  [ML] Re-enable tests muted in #30982
nik9000 added a commit that referenced this pull request Jun 15, 2018
@nik9000
REST Client: NodeSelector for node attributes (#31296)
f03de82 
Add a `NodeSelector` so that users can filter the nodes that receive
requests based on node attributes.

I believe we'll need this to backport #30523 and we want it anyway.

I also added a bash script to help with rebuilding the sniffer parsing
test documents.
dnhatn added a commit that referenced this pull request Jun 19, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
35243f2 
* 6.x:
  Add get stored script and delete stored script to high level REST API
  Increasing skip version for failing test on 6.x
  Skip get_alias tests for 5.x (#31397)
  Fix defaults in GeoShapeFieldMapper output (#31302)
  Test: better error message on failure
  Mute DefaultShardsIT#testDefaultShards test
  Fix reference to XContentBuilder.string() (#31337)
  [DOCS] Adds monitoring breaking change (#31369)
  [DOCS] Adds security breaking change (#31375)
  [DOCS] Backports breaking change (#31373)
  RestAPI: Reject forcemerge requests with a body (#30792)
  Docs: Use the default distribution to test docs (#31251)
  Use system context for cluster state update tasks (#31241)
  [DOCS] Adds testing for security APIs (#31345)
  [DOCS] Removes ML item from release highlights
  [DOCS] Removes breaking change (#31376)
  REST high-level client: add validate query API (#31077)
  Move language analyzers from server to analysis-common module. (#31300)
  Expose lucene's RemoveDuplicatesTokenFilter (#31275)
  [Test] Fix :example-plugins:rest-handler on Windows
  Delete typos in SAML docs (#31199)
  Ensure we don't use a remote profile if cluster name matches (#31331)
  Test: Skip alias tests that failed all weekend
  [DOCS] Fix version in SQL JDBC Maven template
  [DOCS] Improve install and setup section for SQL JDBC
  Add ingest-attachment support for per document `indexed_chars` limit (#31352)
  SQL: Fix rest endpoint names in node stats (#31371)
  [DOCS] Fixes small issue in release notes
  Support for remote path in reindex api Closes #22913
  [ML] Put ML filter API response should contain the filter (#31362)
  Remove trial status info from start trial doc (#31365)
  [DOCS] Added links in breaking changes pages
  [DOCS] Adds links to release notes and highlights
  Docs: Document changes in rest client
  QA: Fix tribe tests to use node selector
  REST Client: NodeSelector for node attributes (#31296)
  LLClient: Fix assertion on windows
  LLClient: Support host selection (#30523)
  Add QA project and fixture based test for discovery-ec2 plugin (#31107)
  [ML] Hold ML filter items in sorted set (#31338)
  [ML] Add description to ML filters (#31330)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna approved these changes

Assignees
No one assigned
Labels
:Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >enhancement v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nik9000 @elasticmachine @javanna @colings86