METRON-1503: Alerts are not getting populated in alerts UI when search engine is Solr #975
Conversation
|
Can this fix be tested in a unit or integration test? |
|
I modified a case in SearchIntegrationTest.java so that the test would have failed before this change but passes now. There is no unit test right now for the SolrSearchDao. I think the original reason was because that class is very well covered by the integration test. Do you think we need one? |
|
Yes, I would argue that integration tests are no replacement for unit tests. Unless there is some reason we cannot unit test this thing more directly. And, if that is the case, then I'd argue we need to refactor. Think of the dread you get when an integration test fails. 'Oh crap, now what happened?' And you get that feeling because it is very difficult to pinpoint root cause when an integration test fails. We really should focus on more direct, targeted testing, unless there is some technical hurdle here. |
|
If I remember correctly, there was a technical challenge writing unit tests for the ElasticsearchDao. The ES client library was difficult to mock which made the tests verbose and difficult to understand. That isn't necessarily true for the Solr client though so I'm happy to take add some unit tests. If we're going to do it we might as well do it right and add tests for the whole class. Are you ok reviewing that in this PR? |
|
I agree. Doing it right is going to save us in the long run. I'd be happy to review it in this PR. If I can help with any of this, just let me know. |
|
The latest commit adds unit tests for all the Solr Dao classes except SolrMetaAlertDao. Mocking some of the Solr client classes (and client libraries in general) was challenging so I opted to move them to their own methods within the Daos and spy on those methods. I feel like this achieves the same goal with less complexity. I also didn't test for cases where exceptions from Solr client classes and just caught and thrown. If you think there is value in doing this it probably won't require that much effort to add them. I ran into this bug while adding these tests: powermock/powermock#731. Incrementing the powermock version we use solved it. |
There was a problem hiding this comment.
This is great @merrimanr. I love all the new tests that you added. Gold star for that!
A few small feedback comments. This is solid though.
| @@ -61,4 +61,20 @@ public void setResults(List<SearchResult> results) { | |||
| public void setFacetCounts(Map<String, Map<String, Long>> facetCounts) { | |||
| this.facetCounts = facetCounts; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
There was a problem hiding this comment.
Would it make sense to throw in a matching hashCode(..) just to avoid any potential future headaches?
| @@ -72,12 +72,12 @@ public SolrDao() { | |||
| @Override | |||
| public void init(AccessConfig config) { | |||
| if (config.getKerberosEnabled()) { | |||
| HttpClientUtil.addConfigurer(new Krb5HttpClientConfigurer()); | |||
| enableKerberos(); | |||
There was a problem hiding this comment.
Thank you for the refactorings like this. Even though small, they help with readability a bunch. Bravo.
| @@ -176,19 +179,23 @@ private SolrQuery buildSearchRequest( | |||
| facetFields.get().forEach(query::addFacetField); | |||
| } | |||
|
|
|||
| String collections = searchRequest.getIndices().stream().collect(Collectors.joining(",")); | |||
| query.set("collection", collections); | |||
| query.set("collection", getCollections(searchRequest.getIndices())); | |||
There was a problem hiding this comment.
This is the key fix for the bug that you found. When I comment this out and run the tests, the integration test does fail, which is great.
Unfortunately, I would have expected the SolrSearchDaoTest to fail too, but it did not. If you think it is easy enough to fail a unit test on this condition, it would be a good add.
|
+1 Good stuff |
… engine is Solr (merrimanr) closes #975
Contributor Comments
Solr throws an error when an index included in a query doesn't exist where Elasticsearch simply ignores that error. This PR makes the Solr implementation equivalent to the Elasticsearch implementation by removing an indices that don't exist from the query before it is executed.
This has been tested on full dev. After spinning it up and switching the Indexing search engine to Solr, alerts should appear in the Alerts UI. I also slightly modified a test case to catch this in the future.
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
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?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.