Skip to content
This repository was archived by the owner on Aug 20, 2025. It is now read-only.

Conversation

@wardbekker
Copy link

@wardbekker wardbekker commented Jun 19, 2017

Contributor Comments

This is a work-in-progress fix for Elasticsearch ES5 with Xshield client support. I'm opening a pull request to work through the remaining issues with the team. To be clear, the aim is discussion only for now, not for actually pulling in the code. @jasper-k verified the patch is working against a ES5.x Xpack enabled cluster.

Open issues in this pull request:

  • To have Storm and Elasticsearch 5.x client happily working together it's needed to have a Storm version that is compiled with log4j-core en log4j-api versions 2.8. This is already merged in the Storm sourcecode (see https://issues.apache.org/jira/browse/STORM-2326), however this fix is not shipping with HDP2.5.x. This fix is shipping withing the HDP 2.6 release.
  • While the metron-elasticsearch unit and integration tests are working, the ElasticsearchDataPrunerTest from metron-datamanagement are not. It can't seem to initialize ElasticSearch classes (e.g. java.lang.NoClassDefFoundError: Could not initialize class org.elasticsearch.common.ParseField). It seems to be related with this issue: transport client fails in uber jar - no shortHash elastic/elasticsearch#21627 . I could use some help with resolving this.
  • The ES server used in the integration test is not using Xpack authentication. I could not find any public reference on creating an x-pack authentication enabled server via their SDK . Xpack authentication is confirmed to work for the Storm Writer bolt, but no integration test can be added until this issue is resolved.
  • I've updated the index template for yaf as used in the integration tests to be ES5.x compatible. Other index templates need the same treatment
  • ES & Kibana Mpacks are not updated.

.put("index.number_of_shards", 1)
.put("node.mode", "network")
.put("index.number_of_replicas", 1);
.put("transport.type", "netty4");
Copy link
Member

Choose a reason for hiding this comment

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

Can we parameterize this?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, nvm, I retract. This is in the integration test component.

Copy link
Member

@cestella cestella left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!


byte[] indexTemplate = new byte[0];
try {
indexTemplate = Files.readAllBytes(Paths.get("/Users/wbekker/metron/metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/files/yaf_index.template"));
Copy link
Member

Choose a reason for hiding this comment

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

This path should be relative.

try {
indexTemplate = Files.readAllBytes(Paths.get("/Users/wbekker/metron/metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/files/yaf_index.template"));
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to throw back up a Runtime Exception

try {
node.close();
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

We either want to throw up the exception or at least log it with a logger, rather than stdout.

@wardbekker
Copy link
Author

wardbekker commented Jun 19, 2017

Thanks for the review @cestella I've pushed a commit fixing the reported issues. (Again, not for merging)

@wardbekker
Copy link
Author

hey @cestella, @simonellistonball, see updated contributor notes. It's not ready for a official pull request, but this gives a good idea on the impact on the code for a working ES5.x implementation.

@JonZeolla
Copy link
Member

@wardbekker Can you please merge master and deconflict?

@wardbekker
Copy link
Author

@JonZeolla done.

@simonellistonball
Copy link
Contributor

Seems like this is failing on some un-related temporary test failures. Can we get Travis kicked, and see what's left to do on this?

@justinleet
Copy link
Contributor

As a note, this ticket is slightly impacted by the metaalerts backend ticket (#734). The alerts field in the various templates should be removed and the search queries for meta alerts updated according to https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html#_ignoring_unmapped_fields, in order to allow for searches against metaalerts without having to have an alert field in each template.

@nickwallen
Copy link
Contributor

This functionality was completed in #840. As mentioned in #840 this inspired much of that work. Is there anything else needed from this PR? If not, can you close this PR @wardbekker ?

Thanks

@wardbekker
Copy link
Author

agreed @nickwallen

@wardbekker wardbekker closed this Feb 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants