METRON-1845 Correct Test Data Load in Elasticsearch Integration Tests #1247

nickwallen · 2018-10-25T19:04:44Z

The Elasticsearch integration tests use the Elasticsearch API to load the test data before running the tests. Loading the test data like this does not accurately reflect how the indices will appear in a production environment.

This should be changed to use our existing ElasticsearchUpdateDao to write the test data. This ensures that any changes made to the 'write' portion of our Elasticsearch code will function correctly with the 'read' portion. Said in a different way, this ensures that telemetry written into Elasticsearch by 'Indexing' can be read correctly by the Alerts UI.

Do not merge until after METRON-1834: Migrate Elasticsearch from TransportClient to new Java REST API #1242. This change is dependent on METRON-1834: Migrate Elasticsearch from TransportClient to new Java REST API #1242. Take a look at the last commit to view the changes specific to this PR.

Testing

This only changes the integration tests. If the integration tests pass, we're golden.

Pull Request Checklist

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

…nsformer Shade plugin transformer.

…ate properties for new client.

nickwallen · 2018-10-25T19:08:19Z

...test/java/org/apache/metron/elasticsearch/integration/components/ElasticSearchComponent.java

-      }
-      bulkRequest.add(indexRequestBuilder);
-    }
+  public void add(UpdateDao updateDao, String indexName, String sensorType, Iterable<String> docs)


The Elasticsearch tests use this method to load test data. Rather than using the Transport client here, we use the UpdateDao to load the test data.

Might it be better to just use IndexDao, which extends UpdateDao, SearchDao, RetrieveLatestDao, ColumnMetadataDao? To that end, if we're looking to route all of this through the ES component in that fashion, it might make sense to simply replace the internal private Client client; and instead use the new desired IndexDao for the proxied calls to ES. It could be setup at construction time of the component and remove the need to do the same thing for every test that uses the component class, unless they actually want to do something custom and pass in their own dao during init.

Might it be better to just use IndexDao, which extends UpdateDao, SearchDao, RetrieveLatestDao, ColumnMetadataDao?

Why would that be better? It needs to do updates, so it needs an UpdateDao. Seemed logical to me.

To that end, if we're looking to route all of this through the ES component in that fashion, it might make sense to simply replace the internal private Client client; and instead use the new desired IndexDao for the proxied calls to ES.

I don't think we're ready to do all that quite yet. There is still some legacy functionality in ElasticSearchComponent that uses the underlying client for the old Admin API. See close, createIndexWithMapping, and start. I would not want to update all that in this PR.

The IndexDao handles all of the plumbing for the init that you're duplicating in this test class.

...est/java/org/apache/metron/elasticsearch/integration/ElasticsearchSearchIntegrationTest.java

...form/metron-indexing/src/test/java/org/apache/metron/indexing/dao/UpdateIntegrationTest.java

nickwallen · 2018-10-25T19:15:48Z

...est/java/org/apache/metron/elasticsearch/integration/ElasticsearchSearchIntegrationTest.java

+    es.add(updateDao, snortIndex, "snort", snortDocuments);
+
+    // wait until the test documents are visible
+    assertEventually(() -> Assert.assertEquals(10, findAll(searchDao).getTotal()));


Unless I added this to hold up the test and wait for the tests data to be 'visible', some of the tests would reliably fail because the test data wasn't loaded.

@nickwallen is it possible to parameterize the call to do something similar to this? https://github.com/apache/metron/pull/1242/files#diff-99b67fd77500d6232462dc7c27ecff67R148

It's possible this is only available on batch calls, but I wanted to mention it in case we can get away with the assertEventually calls.

I have tried to do exactly this on a private branch that is closer to the code base in #1269. I made the integration tests supply a RefreshPolicy that was supposed to hold them up until the loaded data is discoverable/searchable.

Unfortunately, I was never able to get that to work consistently despite what the ES docs say. I still needed the assertEventually for the integration tests. I would have liked that to work.

Are you able to get this to fail consistently? I ran it a number of times and couldn't get it to fail. I'm wondering if there are any other interesting entries in the logs when this occurs. These tests from ES might be of help - https://github.com/elastic/elasticsearch/pull/17986/files#diff-1c9f982dbd2f9ddb2853d135884621b5R112

Are you able to get this to fail consistently? I ran it a number of times and couldn't get it to fail.

Can you describe what you did? You just removed the call to assertEventually (now line 162) and the tests continue to work reliably?

I meant on the es client change PR, not on this PR. Yes, I had removed assertEventually calls and it consistently passed for me in the search integration test.

This reverts commit 7d9ee25.

nickwallen · 2018-11-16T15:39:46Z

I have merged this with master to pull in the changes from #1242. This is ready for review.

.../metron-solr/src/test/java/org/apache/metron/solr/integration/SolrUpdateIntegrationTest.java

...est/java/org/apache/metron/elasticsearch/integration/ElasticsearchSearchIntegrationTest.java

mmiklavc · 2018-11-27T05:10:57Z

...test/java/org/apache/metron/elasticsearch/integration/components/ElasticSearchComponent.java

-      }
-      bulkRequest.add(indexRequestBuilder);
-    }
+  public void add(UpdateDao updateDao, String indexName, String sensorType, Iterable<String> docs)


Might it be better to just use IndexDao, which extends UpdateDao, SearchDao, RetrieveLatestDao, ColumnMetadataDao? To that end, if we're looking to route all of this through the ES component in that fashion, it might make sense to simply replace the internal private Client client; and instead use the new desired IndexDao for the proxied calls to ES. It could be setup at construction time of the component and remove the need to do the same thing for every test that uses the component class, unless they actually want to do something custom and pass in their own dao during init.

.../metron-solr/src/test/java/org/apache/metron/solr/integration/SolrUpdateIntegrationTest.java

…y loaded into Elasticsearch and not HBase, which matches the previous behavior

mmiklavc · 2018-11-28T18:44:10Z

As a side note, I tend to like the approach of using the DAO layers to read/write the way you've done here @nickwallen . I've used this approach in the past, and it helped with test coverage and simplifying the amount of extra custom test code required. It might be worth an overall discussion on how we want to architect testing other similar endpoints going forward.

…h to write test data, it now maintains its own. This simplifies usage of the ElasticsearchComponent by the integration tests

mmiklavc · 2018-11-28T20:05:45Z

lgtm @nickwallen, +1 pending Travis

…ation tests to define a refresh policy

mmiklavc · 2018-11-28T20:42:41Z

Still +1 on latest commit - that's a nice improvement. Glad the retry policy ended up working out!

cestella and others added 12 commits October 8, 2018 18:06

Casey Stella - elasticsearch rest client migration base work

a7c7dc2

Update shade plugin version

10410ea

Fix es update dao test

a33a168

Merge with master. Fix es search integration tests

52c3c96

Merge branch 'master' into es-rest-client

4742832

Get shade plugin working with the new ES client and the ClassIndexTra…

4380996

…nsformer Shade plugin transformer.

Introduce config classes for managing ES client configuration. Transl…

af03f6f

…ate properties for new client.

Resolve merge conflicts with master

1c8eac2

Remove extra deps in metron-elasticsearch around log4j.

1a47ded

Fixes for dep version issues.

54870d6

METRON-1845 Correct Test Data Load in Elasticsearch Integration Tests

554de87

Removed dead function

3b95d1e

nickwallen commented Oct 25, 2018

View reviewed changes

nickwallen added 3 commits October 26, 2018 14:55

Part way through Solr changes that I am going to roll-back

7d9ee25

Revert "Part way through Solr changes that I am going to roll-back"

e699386

This reverts commit 7d9ee25.

Resolved integration test differences with Solr

4a7a837

This was referenced Nov 5, 2018

METRON-1849 Elasticsearch Index Write Functionality Should be Shared #1254

Closed

METRON-1834: Migrate Elasticsearch from TransportClient to new Java REST API #1242

Closed

Merge remote-tracking branch 'apache/master' into METRON-1845

a45f800

nickwallen mentioned this pull request Nov 13, 2018

METRON-1867 Remove /api/v1/update/replace endpoint #1259

Closed

12 tasks

nickwallen added 2 commits November 15, 2018 10:32

Merge remote-tracking branch 'apache/master' into METRON-1845

cbc4f5a

Merge branch 'master' into METRON-1845

401e314

nickwallen mentioned this pull request Nov 16, 2018

METRON-1879 Allow Elasticsearch to Auto-Generate the Document ID #1269

Closed

12 tasks

justinleet mentioned this pull request Nov 19, 2018

METRON-1877: Nested IF ELSE statements can cause parse errors in Stellar #1268

Closed

10 tasks

mmiklavc reviewed Nov 27, 2018

View reviewed changes

nickwallen added 2 commits November 27, 2018 12:16

Fixed the ElasticsearchUpdateIntegrationTest so that test data is onl…

50fd7ad

…y loaded into Elasticsearch and not HBase, which matches the previous behavior

Simlified initialization of the DAOs needed to load test data

76c6662

Instead of the ElasticsearchComponent being passed in a DAO with whic…

789dd7e

…h to write test data, it now maintains its own. This simplifies usage of the ElasticsearchComponent by the integration tests

nickwallen added 2 commits November 28, 2018 15:34

Able to remove the need for assertEventually() by allowing the integr…

043d805

…ation tests to define a refresh policy

Removed some dead code

dfdb2da

Merge remote-tracking branch 'apache/master' into METRON-1845

e7c7db3

asfgit closed this in 89a2bed Nov 29, 2018

METRON-1845 Correct Test Data Load in Elasticsearch Integration Tests #1247

METRON-1845 Correct Test Data Load in Elasticsearch Integration Tests #1247

Uh oh!

Conversation

nickwallen commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen commented Nov 16, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mmiklavc commented Nov 28, 2018

Uh oh!

mmiklavc commented Nov 28, 2018

Uh oh!

mmiklavc commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickwallen commented Oct 25, 2018 •

edited

Loading

nickwallen Nov 27, 2018 •

edited

Loading

nickwallen Nov 26, 2018 •

edited

Loading

nickwallen Nov 27, 2018 •

edited

Loading