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

Adding mapping for hostname field #37288

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Jan 10, 2019

This new hostname field is meant to be a replacement for its sibling name field. See elastic/beats#9943, particularly elastic/beats#9943 (comment).

This PR simply adds the new field (hostname) to the mapping without removing the old one (name), because a user might be running an older-version Beat (without this field rename in it) with a newer-version Monitoring ES cluster (with this PR's change in it).

AFAICT the Monitoring UI isn't currently using the name field so no changes are necessary there yet. If it decides to start using the name field, it will also want to look at the value of the hostname field.

This new field is meant to be a replacement for its sibling name field. See elastic/beats#9943.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

jenkins, test this

@@ -52,6 +52,9 @@
"name": {
"type": "keyword"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thought I had was to add "copy_to": "hostname" here, for the case of an older-version Beat (sending in name) working with a newer-version ES. Thoughts, reviewers?

Copy link
Member

Choose a reason for hiding this comment

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

Both fields can exist in newer version with potentially different values. So better not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize that. I thought hostname was a replacement for the name field. If that's not the case, then it makes sense not to copy from name -> hostname.

Copy link
Member

Choose a reason for hiding this comment

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

You have a point here. For the monitoring data reporting it is actually a replacement / renaming. I was thinking of the data shipped by Beats.

@ycombinator
Copy link
Contributor Author

jenkins, test this

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

LGTM. As a side note, the version of the templates will need to be increased in the backport to 6.x.

@ycombinator ycombinator merged commit b86621c into elastic:master Jan 14, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 15, 2019
* master: (28 commits)
  Introduce retention lease serialization (elastic#37447)
  Update Delete Watch to allow unknown fields (elastic#37435)
  Make finalize step of recovery source non-blocking (elastic#37388)
  Update the default for include_type_name to false. (elastic#37285)
  Security: remove SSL settings fallback (elastic#36846)
  Adding mapping for hostname field (elastic#37288)
  Relax assertSameDocIdsOnShards assertion
  Reduce recovery time with compress or secure transport (elastic#36981)
  Implement ccr file restore (elastic#37130)
  Fix Eclipse specific compilation issue (elastic#37419)
  Performance fix. Reduce deprecation calls for the same bulk request (elastic#37415)
  [ML] Use String rep of Version in map for serialisation (elastic#37416)
  Cleanup Deadcode in Rest Tests (elastic#37418)
  Mute IndexShardRetentionLeaseTests.testCommit elastic#37420
  unmuted test
  Remove unused index store in directory service
  Improve CloseWhileRelocatingShardsIT (elastic#37348)
  Fix ClusterBlock serialization and Close Index API logic after backport to 6.x (elastic#37360)
  Update the scroll example in the docs (elastic#37394)
  Update analysis.asciidoc (elastic#37404)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants