You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by merrimanr <gi...@git.apache.org> on 2018/09/07 21:18:22 UTC
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
GitHub user merrimanr opened a pull request:
https://github.com/apache/metron/pull/1190
METRON-1771: Update REST endpoints to support eventually consistent UI updates
## Contributor Comments
This PR updates several REST endpoints to return the state of an object on update operations. These endpoints include:
- MetaAlertController
- create
- addAlertsToMetaAlert
- removeAlertsFromMetaAlert
- updateMetaAlertStatus
- UpdateController
- patch
- replace
- addCommentToAlert
- removeCommentFromAlert
The general approach is to change the return type of the various Dao methods that update objects to return the updated document. In most cases this was straightforward but did require small changes in several files due to how the Dao classes are composed and the various implementations that exist (ES, Solr, InMemory, etc).
Perhaps the most significant change was to the InMemoryMetaAlertDao classes used for REST integration testing because they were no longer sufficient after the interface change. Before they stored a mapping between metaalerts and child alerts which worked fine when just returning a true/false on adding/removing child alerts. With this PR the InMemoryMetaAlertDao classes now must return the full object. As I worked through updating these methods to do this, I found myself rewriting the methods in AbstractLuceneMetaAlertUpdateDao. Instead of doing that, I refactored the InMemoryMetaAlertDao classes to more closely match how the MetaAlertDao classes are composed and was able to reuse the implementations in AbstractLuceneMetaAlertUpdateDao. I feel this makes the InMemoryMetaAlertDao classes smaller and easier to maintain moving forward.
### Changes Included
- Updated the various *UpdateDao interfaces and implementations to return Documents
- Updated the REST services and controllers to match the new interfaces
- Changed the InMemoryDao classes as described above
- Updated the various unit and integration tests to verify that correct documents are returned (includes new tests in several cases)
- Updated and added documentation as needed
- Fixed a bug in HBaseDaoIntegrationTest.java where tables were not being cleared after each test
### Testing
In addition to unit and integration tests passing, I have tested this in full dev with Elasticsearch. The testing approach is the same for each endpoint listed above: send a request and verify the document that is returned includes the expected change. Here are a couple of examples
#### Testing the patch endpoint
Send a patch request:
```
curl -X PATCH --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "1881af3c-88df-4e72-8e81-34538b6e774b",
"patch": [
{
"op":"add",
"path":"/newField",
"value":"newValue"
}
],
"sensorType": "bro"
}' 'http://node1:8082/api/v1/update/patch'
```
The response should contain the full document along with the update, in this case a `newField` field added with a value of `newValue`:
```
{
"timestamp": 1536185190979,
"document": {
"bro_timestamp": "1536182284.908185",
"ip_dst_port": 8080,
...
"newField": "newValue"
},
"guid": "1881af3c-88df-4e72-8e81-34538b6e774b",
"sensorType": "bro"
}
```
#### Testing the metaalert create endpoint:
Create a metaalert:
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "0588e788-3a91-4113-94a0-d80daec00d05",
"sensorType": "snort"
}
],
"groups": [
"group1"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
This should return the full metaalert along with the child alert:
```
{
"timestamp": 1536203159546,
"document": {
"average": 10,
"max": 10,
"metron_alert": [
{
"msg": "'snort test alert'",
...
"metaalerts": [
"f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e"
]
}
],
"threat:triage:score": 10,
"count": 1,
"groups": [
"group1"
],
"sum": 10,
"source:type": "metaalert",
"min": 10,
"median": 10,
"guid": "f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e",
"timestamp": 1536203159546,
"status": "active"
},
"guid": "f65e4d0e-bdad-4ac7-ab22-4cf207c3fb2e",
"sensorType": "metaalert"
}
```
I also tested every endpoint using the Alerts UI. Everything should continue working as expected.
### Outstanding items
There are a couple of changes I'm not quite sure about and would like some input:
- Are the changes made to the InMemoryUpdateDao classes reasonable?
- The case of missing documents are handled in 2 different ways as far as I can tell. Either a null is returned or an empty Optional. I just happened to see the null approach first so that's the pattern I followed. However I think we may want to make this consistent and the Optional empty approach feels more appropriate to me. What do you think?
- In the MultiIndexDao, updates are performed in parallel across the different Daos. Since we're assuming the updates should all be identical I'm returning the first one as the return value. Is there a better approach?
I've made some comments inline in this PR to highlight these and provide some context.
Fortunately I did not have to make any UI changes since the return value of these endpoints is ignored in most cases. We should have a follow on task to properly update the client side code to match these new endpoints. I believe there are a couple outstanding bugs related to displaying inconsistent results in the UI after updates.
I am still working on testing with Solr and adding more code comments to add some clarity around changes that may not be obvious.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) 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:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [x] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified and tested manually?
- [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [x] Have you written or updated unit tests and or integration tests to verify your changes?
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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`:
```
cd site-book
mvn site
```
#### 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](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/merrimanr/incubator-metron METRON-1771
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1190.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1190
----
commit 39ef134852f1ce8626e77413381ccc4779bbcad8
Author: merrimanr <me...@...>
Date: 2018-09-07T20:18:48Z
initial commit
----
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216092662
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/integration/HBaseDaoIntegrationTest.java ---
@@ -61,8 +60,8 @@
0x54,0x79,0x70,0x65
};
- @BeforeClass
- public static void startHBase() throws Exception {
+ @Before
--- End diff --
Tables need to be cleared after each test.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216733487
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -121,20 +131,30 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
if (exceptions.size() > 0) {
throw new IOException(Joiner.on("\n").join(exceptions));
}
+ return newVersions.get(0);
}
@Override
- public void removeCommentFromAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = getLatest(request.getGuid(), request.getSensorType());
- removeCommentFromAlert(request, latest);
+ return removeCommentFromAlert(request, latest);
}
+ /**
+ * Removes comments from an alert. Updates are written to each Dao in parallel with the assumption that all updates
+ * are identical. The first update to be applied is returned as the current version of the alert with comments removed.
+ * @param request Request to remove comments
+ * @param latest The latest version of the alert the comments will be removed from.
+ * @return The complete alert document with comments removed.
+ * @throws IOException
+ */
@Override
- public void removeCommentFromAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ final List<Document> newVersions = new ArrayList<>();
--- End diff --
Same here... If we do it this way, we just need to make this a thread-safe list.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216733316
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -121,20 +131,30 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
if (exceptions.size() > 0) {
throw new IOException(Joiner.on("\n").join(exceptions));
}
+ return newVersions.get(0);
--- End diff --
I don't think we can safely add the Documents to `newVersions` since it is not a thread-safe list.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222823946
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
Looks good to me. Latest commit includes this change.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216726476
--- Diff: metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDao.java ---
@@ -133,21 +135,21 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
Document newVersion = new Document(latest);
newVersion.getDocument().put(COMMENTS_FIELD, originalComments);
- update(newVersion, Optional.empty());
+ return update(newVersion, Optional.empty());
}
@Override
@SuppressWarnings("unchecked")
- public void removeCommentFromAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = retrieveLatestDao.getLatest(request.getGuid(), request.getSensorType());
- removeCommentFromAlert(request, latest);
+ return removeCommentFromAlert(request, latest);
}
@Override
@SuppressWarnings("unchecked")
- public void removeCommentFromAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
if (latest == null) {
- return;
+ return null;
--- End diff --
Same comment here. We should never see a request to remove a comment from a doc that doesn't exist. So let's throw an exception with a message that we can use to debug the problem.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217459852
--- Diff: metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.elasticsearch.dao;
+
+import org.apache.metron.indexing.dao.AccessConfig;
+import org.apache.metron.indexing.dao.UpdateDaoTest;
+import org.apache.metron.indexing.dao.update.UpdateDao;
+import org.elasticsearch.client.transport.TransportClient;
+import org.junit.Before;
+
+import static org.mockito.Mockito.mock;
+
+public class ElasticsearchUpdateDaoTest extends UpdateDaoTest {
--- End diff --
Check out UpdateDaoTest. As I created tests for ElasticsearchUpdateDao, SolrUpdateDao, and HBaseDao I found myself copy/pasting the various tests. So I consolidated them into a single test. I believe there is value in having a single sets of tests for the UpdateDao interface methods that all the Daos must satisfy. This class sets up it's Dao implementation to be used in the tests.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222470180
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
No problem, don't mind working it out here and now. The case that causes the test to fail (although I'm not sure that was the intention of the test) is where 2 DAOs are contained in a MultiIndexDao and the first DAO returns an invalid result (no document or exception) while the second returns a valid result. I don't know what would cause a document to be missing both a document and an exception.
The question is what should happen when this unexpected condition does happen? Should it blow up and throw and exception or return a document if at least one DAO returns a valid one.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216091258
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/InMemoryMetaAlertDao.java ---
@@ -174,145 +193,29 @@ public SearchResponse getAllMetaAlertsForAlert(String guid) throws InvalidSearch
@SuppressWarnings("unchecked")
@Override
- public MetaAlertCreateResponse createMetaAlert(MetaAlertCreateRequest request)
- throws InvalidCreateException {
- List<GetRequest> alertRequests = request.getAlerts();
- if (alertRequests.isEmpty()) {
- MetaAlertCreateResponse response = new MetaAlertCreateResponse();
- response.setCreated(false);
- return response;
- }
- // Build meta alert json. Give it a reasonable GUID
- JSONObject metaAlert = new JSONObject();
- String metaAlertGuid =
- "meta_" + (InMemoryDao.BACKING_STORE.get(getMetaAlertIndex()).size() + 1);
- metaAlert.put(GUID, metaAlertGuid);
-
- JSONArray groupsArray = new JSONArray();
- groupsArray.addAll(request.getGroups());
- metaAlert.put(MetaAlertConstants.GROUPS_FIELD, groupsArray);
-
- // Retrieve the alert for each guid
- // For the purpose of testing, we're just using guids for the alerts field and grabbing the scores.
- JSONArray alertArray = new JSONArray();
- List<Double> threatScores = new ArrayList<>();
- Collection<String> alertGuids = new ArrayList<>();
- for (GetRequest alertRequest : alertRequests) {
- SearchRequest searchRequest = new SearchRequest();
- searchRequest.setIndices(ImmutableList.of(alertRequest.getIndex().get()));
- searchRequest.setQuery("guid:" + alertRequest.getGuid());
- try {
- SearchResponse searchResponse = search(searchRequest);
- List<SearchResult> searchResults = searchResponse.getResults();
- if (searchResults.size() > 1) {
- throw new InvalidCreateException(
- "Found more than one result for: " + alertRequest.getGuid() + ". Values: "
- + searchResults
- );
- }
-
- if (searchResults.size() == 1) {
- SearchResult result = searchResults.get(0);
- alertArray.add(result.getSource());
- Double threatScore = Double
- .parseDouble(
- result.getSource().getOrDefault(MetaAlertConstants.THREAT_FIELD_DEFAULT, "0")
- .toString());
-
- threatScores.add(threatScore);
- }
- } catch (InvalidSearchException e) {
- throw new InvalidCreateException("Unable to find guid: " + alertRequest.getGuid(), e);
- }
- alertGuids.add(alertRequest.getGuid());
- }
-
- metaAlert.put(MetaAlertConstants.ALERT_FIELD, alertArray);
- metaAlert.putAll(new MetaScores(threatScores).getMetaScores());
- metaAlert.put(MetaAlertConstants.STATUS_FIELD, MetaAlertStatus.ACTIVE.getStatusString());
-
- // Add the alert to the store, but make sure not to overwrite existing results
- InMemoryDao.BACKING_STORE.get(getMetaAlertIndex()).add(metaAlert.toJSONString());
-
- METAALERT_STORE.put(metaAlertGuid, new HashSet<>(alertGuids));
-
- MetaAlertCreateResponse createResponse = new MetaAlertCreateResponse();
- createResponse.setGuid(metaAlertGuid);
- createResponse.setCreated(true);
- return createResponse;
+ public Document createMetaAlert(MetaAlertCreateRequest request)
+ throws InvalidCreateException, IOException {
+ return metaAlertUpdateDao.createMetaAlert(request);
--- End diff --
Now we're delegating to the AbstractLuceneMetaAlertUpdateDao class instead of duplicating it.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222000643
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
Upon further review, I'm not sure this is what we want. This caused an integration test to fail because the result from the first DAO did not contain a document or exception while the result from the second DAO did contain a document. Before this change, the document from the second DAO would have been returned. I think we want to return a document if possible, even if all DAOs are not consistent. Do you disagree?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216735553
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/metaalert/lucene/AbstractLuceneMetaAlertUpdateDao.java ---
@@ -170,21 +169,51 @@ protected Document buildCreateDocument(Iterable<Document> alerts, List<String> g
return updates;
}
+ /**
+ * Adds alerts to a metaalert, based on a list of GetRequests provided for retrieval.
+ * @param metaAlertGuid The GUID of the metaalert to be given new children.
+ * @param alertRequests GetRequests for the appropriate alerts to add.
+ * @return The updated metaalert with alerts added.
+ */
+ @Override
+ public Document addAlertsToMetaAlert(String metaAlertGuid, List<GetRequest> alertRequests)
+ throws IOException {
+
+ Document metaAlert = retrieveLatestDao
+ .getLatest(metaAlertGuid, MetaAlertConstants.METAALERT_TYPE);
+ if (MetaAlertStatus.ACTIVE.getStatusString()
+ .equals(metaAlert.getDocument().get(MetaAlertConstants.STATUS_FIELD))) {
+ Iterable<Document> alerts = retrieveLatestDao.getAllLatest(alertRequests);
+ Map<Document, Optional<String>> updates = buildAddAlertToMetaAlertUpdates(metaAlert, alerts);
+ update(updates);
+ return metaAlert;
+ } else {
+ throw new IllegalStateException("Adding alerts to an INACTIVE meta alert is not allowed");
+ }
+ }
+
+ /**
+ * Removes alerts from a metaalert, based on a list of GetRequests provided for retrieval.
+ * @param metaAlertGuid The GUID of the metaalert to remove children from.
+ * @param alertRequests A list of GetReqests that will provide the alerts to remove
+ * @return The updated metaalert with alerts removed.
+ * @throws IllegalStateException If the metaalert is inactive.
+ */
@Override
@SuppressWarnings("unchecked")
- public boolean removeAlertsFromMetaAlert(String metaAlertGuid, List<GetRequest> alertRequests)
- throws IOException {
+ public Document removeAlertsFromMetaAlert(String metaAlertGuid, List<GetRequest> alertRequests)
+ throws IOException, IllegalStateException {
Document metaAlert = retrieveLatestDao
.getLatest(metaAlertGuid, MetaAlertConstants.METAALERT_TYPE);
if (metaAlert == null) {
- return false;
+ return null;
--- End diff --
Should we also treat this as an exceptional condition?
---
[GitHub] metron issue #1190: METRON-1771: Update REST endpoints to support eventually...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1190
@nickwallen, the latest commit should address your comments. Let me know what you think.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217441386
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
Do we gain anything from a slight restructuring of your if/else chain that adds a sanity check?
```
if(dc.getException().isPresent()) {
..
} else if(dc.getDocument.isPresent()) {
..
} else {
throw new IllegalStateException("Expected either document or exception; got neither");
}
```
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216090450
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -121,20 +131,30 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
if (exceptions.size() > 0) {
throw new IOException(Joiner.on("\n").join(exceptions));
}
+ return newVersions.get(0);
--- End diff --
Here we are collecting each updated document in a list. In theory they are all the same so it shouldn't matter which one we return. Is there a better way to do this?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217439809
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -101,48 +103,59 @@ public void batchUpdate(Map<Document, Optional<String>> updates) throws IOExcept
}
@Override
- public void addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = getLatest(request.getGuid(), request.getSensorType());
- addCommentToAlert(request, latest);
+ return addCommentToAlert(request, latest);
}
-
+ /**
+ * Adds comments to an alert. Updates are written to each Dao in parallel with the assumption that all updates
+ * are identical. The first update to be applied is returned as the current version of the alert with comments added.
+ * @param request Request to add comments
+ * @param latest The latest version of the alert the comments will be added to.
+ * @return The complete alert document with comments added.
+ * @throws IOException
+ */
@Override
- public void addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
- List<String> exceptions =
- indices.parallelStream().map(dao -> {
- try {
- dao.addCommentToAlert(request, latest);
- return null;
- } catch (Throwable e) {
- return dao.getClass() + ": " + e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e);
- }
- }).filter(Objects::nonNull).collect(Collectors.toList());
- if (exceptions.size() > 0) {
- throw new IOException(Joiner.on("\n").join(exceptions));
- }
+ public Document addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ List<DocumentContainer> output =
+ indices.parallelStream().map(dao -> {
+ try {
+ return new DocumentContainer(dao.addCommentToAlert(request, latest));
+ } catch (Throwable e) {
+ return new DocumentContainer(e);
+ }
+ }).collect(Collectors.toList());
--- End diff --
Nice! I like your approach.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216090059
--- Diff: metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDao.java ---
@@ -108,20 +109,21 @@ public void batchUpdate(Map<Document, Optional<String>> updates) throws IOExcept
throw new IOException(
"ElasticsearchDao upsert failed: " + bulkResponse.buildFailureMessage());
}
+ return updates;
}
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = retrieveLatestDao.getLatest(request.getGuid(), request.getSensorType());
- addCommentToAlert(request, latest);
+ return addCommentToAlert(request, latest);
}
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
if (latest == null) {
- return;
+ return null;
--- End diff --
This is an example of returning null when an document is not found. Would Optional.empty() be better?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222421545
--- Diff: metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.elasticsearch.dao;
+
+import org.apache.metron.indexing.dao.AccessConfig;
+import org.apache.metron.indexing.dao.UpdateDaoTest;
+import org.apache.metron.indexing.dao.update.UpdateDao;
+import org.elasticsearch.client.transport.TransportClient;
+import org.junit.Before;
+
+import static org.mockito.Mockito.mock;
+
+public class ElasticsearchUpdateDaoTest extends UpdateDaoTest {
--- End diff --
I think you missed the javadoc here.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216739553
--- Diff: metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrUpdateDao.java ---
@@ -172,7 +174,7 @@ public void removeCommentFromAlert(CommentAddRemoveRequest request, Document lat
// Can't remove anything if there's nothing there
if (commentMap == null) {
LOG.debug("Provided alert had no comments to be able to remove from");
- return;
+ return null;
--- End diff --
Should we also treat this as an exceptional condition too?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216090740
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/metaalert/lucene/AbstractLuceneMetaAlertUpdateDao.java ---
@@ -170,21 +169,51 @@ protected Document buildCreateDocument(Iterable<Document> alerts, List<String> g
return updates;
}
+ /**
+ * Adds alerts to a metaalert, based on a list of GetRequests provided for retrieval.
+ * @param metaAlertGuid The GUID of the metaalert to be given new children.
+ * @param alertRequests GetRequests for the appropriate alerts to add.
+ * @return The updated metaalert with alerts added.
+ */
+ @Override
+ public Document addAlertsToMetaAlert(String metaAlertGuid, List<GetRequest> alertRequests)
--- End diff --
This was moved from the Elasticsearch Dao implementation essentially making it the default method for adding alerts to metaalerts (the logic in the Solr implementation is different). This allows us to reuse this code in the InMemory implementation.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216739478
--- Diff: metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrUpdateDao.java ---
@@ -149,21 +151,21 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
Document newVersion = new Document(latest);
newVersion.getDocument().put(COMMENTS_FIELD, commentStrs);
- update(newVersion, Optional.empty());
+ return update(newVersion, Optional.empty());
}
@Override
- public void removeCommentFromAlert(CommentAddRemoveRequest request)
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request)
throws IOException {
Document latest = retrieveLatestDao.getLatest(request.getGuid(), request.getSensorType());
- removeCommentFromAlert(request, latest);
+ return removeCommentFromAlert(request, latest);
}
@Override
- public void removeCommentFromAlert(CommentAddRemoveRequest request, Document latest)
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request, Document latest)
throws IOException {
if (latest == null) {
- return;
+ return null;
--- End diff --
Should we also treat this as an exceptional condition too?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222750671
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
> The question is what should happen when this unexpected condition does happen? Should it blow up and throw and exception or return a document if at least one DAO returns a valid one.
Ah, OK. I'm groking it now. We want it to return null; at least based on the javadocs defined in `RetrieveLatestDao.getLatest` and other places as null means nothing was found. So functionally, we don't want it to throw that exception.
But IMO it might benefit from some clarity. I would suggest changing `getDocument` to `getLatestDocument` with some commentary and clarity on what it means if there is no Document or exception and what happens if there is an error in one of the underlying indices.
Feel free to tweak to your liking or disregard, but this is what I would do.
```
/**
* Returns the most recent {@link Document} from a list of {@link DocumentContainer}s.
*
* @param documentContainers A list of containers; each retrieved from a separate index.
* @return The latest {@link Document} found.
* @throws IOException If any of the {@link DocumentContainer}s contain an exception.
*/
private Document getLatestDocument(List<DocumentContainer> documentContainers) throws IOException {
Document latestDocument = null;
List<String> error = new ArrayList<>();
for(DocumentContainer dc : documentContainers) {
if(dc.getException().isPresent()) {
// collect each exception; multiple can occur, one in each index
Throwable e = dc.getException().get();
error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
} else if(dc.getDocument().isPresent()) {
Document d = dc.getDocument().get();
// is this the latest document so far?
if(latestDocument == null || latestDocument.getTimestamp() < d.getTimestamp()) {
latestDocument = d;
}
} else {
// no document was found in the index
latestDocument = null;
}
}
if(error.size() > 0) {
// report all of the errors encountered
throw new IOException(Joiner.on("\n").join(error));
}
return latestDocument;
}
```
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216728030
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java ---
@@ -309,28 +311,28 @@ public void addCommentToAlert(CommentAddRemoveRequest request, Document latest)
Document newVersion = new Document(latest);
newVersion.getDocument().put(COMMENTS_FIELD, commentsMap);
- update(newVersion, Optional.empty());
+ return update(newVersion, Optional.empty());
}
@Override
@SuppressWarnings("unchecked")
- public void removeCommentFromAlert(CommentAddRemoveRequest request)
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request)
throws IOException {
Document latest = getLatest(request.getGuid(), request.getSensorType());
- removeCommentFromAlert(request, latest);
+ return removeCommentFromAlert(request, latest);
}
@Override
@SuppressWarnings("unchecked")
- public void removeCommentFromAlert(CommentAddRemoveRequest request, Document latest)
+ public Document removeCommentFromAlert(CommentAddRemoveRequest request, Document latest)
throws IOException {
if (latest == null || latest.getDocument() == null) {
throw new IOException("Unable to remove comment document that doesn't exist");
}
List<Map<String, Object>> commentMap = (List<Map<String, Object>>) latest.getDocument().get(COMMENTS_FIELD);
// Can't remove anything if there's nothing there
if (commentMap == null) {
- return;
+ return null;
--- End diff --
Should we also treat this as an exceptional condition?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216092562
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/metaalert/MetaAlertIntegrationTest.java ---
@@ -985,6 +1024,30 @@ protected boolean findCreatedDocs(List<GetRequest> getRequests)
throw new OriginalNotFoundException("Count not find guids after " + MAX_RETRIES + "tries");
}
+ @SuppressWarnings("unchecked")
+ protected void assertEquals(Map<String, Object> expected, Map<String, Object> actual) {
--- End diff --
This method is needed to handle type mismatches (Integer to Long and Float to Double) and child alert ordering.
These fields are not defined in the metaalert template so ES is guessing the type. When read back some field types are different vs when they were written.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216727723
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java ---
@@ -280,14 +282,14 @@ protected Put buildPut(Document update) throws IOException {
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = getLatest(request.getGuid(), request.getSensorType());
- addCommentToAlert(request, latest);
+ return addCommentToAlert(request, latest);
}
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
if (latest == null || latest.getDocument() == null) {
throw new IOException("Unable to add comment to document that doesn't exist");
}
--- End diff --
This seems to match what I am suggesting we do in the Elasticsearch implementation. If we don't have a document, we should thrown an exception.
I'd also argue that the error message provides no useful information to help debug the problem. But that is a pre-existing condition. Would be nice to clean-up though.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216091611
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/InMemoryMetaAlertUpdateDao.java ---
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.indexing.dao;
+
+import org.apache.metron.indexing.dao.metaalert.MetaAlertConfig;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertConstants;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertCreateRequest;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertRetrieveLatestDao;
+import org.apache.metron.indexing.dao.metaalert.lucene.AbstractLuceneMetaAlertUpdateDao;
+import org.apache.metron.indexing.dao.search.GetRequest;
+import org.apache.metron.indexing.dao.search.InvalidCreateException;
+import org.apache.metron.indexing.dao.update.CommentAddRemoveRequest;
+import org.apache.metron.indexing.dao.update.Document;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+
+public class InMemoryMetaAlertUpdateDao extends AbstractLuceneMetaAlertUpdateDao {
--- End diff --
Extending the AbstractLuceneMetaAlertUpdateDao class lets us reuse code from that implementation.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216739437
--- Diff: metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrUpdateDao.java ---
@@ -117,18 +118,19 @@ public void batchUpdate(Map<Document, Optional<String>> updates) throws IOExcept
} catch (SolrServerException e) {
throw new IOException(e);
}
+ return updates;
}
@Override
- public void addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = retrieveLatestDao.getLatest(request.getGuid(), request.getSensorType());
- addCommentToAlert(request, latest);
+ return addCommentToAlert(request, latest);
}
@Override
- public void addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
if (latest == null) {
- return;
+ return null;
--- End diff --
Should we also treat this as an exceptional condition too?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217441951
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/InMemoryMetaAlertRetrieveLatestDao.java ---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.indexing;
+
+import org.apache.metron.indexing.dao.IndexDao;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertRetrieveLatestDao;
+import org.apache.metron.indexing.dao.search.GetRequest;
+import org.apache.metron.indexing.dao.update.Document;
+
+import java.io.IOException;
+import java.util.List;
+
+public class InMemoryMetaAlertRetrieveLatestDao implements MetaAlertRetrieveLatestDao {
--- End diff --
Would you mind adding this comment to the javadoc for this class? I can imagine I will ask myself that question a few years down the road.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217462087
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/InMemoryMetaAlertRetrieveLatestDao.java ---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.indexing;
+
+import org.apache.metron.indexing.dao.IndexDao;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertRetrieveLatestDao;
+import org.apache.metron.indexing.dao.search.GetRequest;
+import org.apache.metron.indexing.dao.update.Document;
+
+import java.io.IOException;
+import java.util.List;
+
+public class InMemoryMetaAlertRetrieveLatestDao implements MetaAlertRetrieveLatestDao {
--- End diff --
Sure no problem.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216725905
--- Diff: metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDao.java ---
@@ -108,20 +109,21 @@ public void batchUpdate(Map<Document, Optional<String>> updates) throws IOExcept
throw new IOException(
"ElasticsearchDao upsert failed: " + bulkResponse.buildFailureMessage());
}
+ return updates;
}
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request) throws IOException {
Document latest = retrieveLatestDao.getLatest(request.getGuid(), request.getSensorType());
- addCommentToAlert(request, latest);
+ return addCommentToAlert(request, latest);
}
@Override
@SuppressWarnings("unchecked")
- public void addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
+ public Document addCommentToAlert(CommentAddRemoveRequest request, Document latest) throws IOException {
if (latest == null) {
- return;
+ return null;
--- End diff --
The `latest` would only be null if there was a request to comment on a non-existent meta-alert, correct? If that is the case, then this *should* never happen. Wouldn't it be better to throw an exception with an appropriate error message so we can investigate the thing that should not have happened?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216735006
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/metaalert/lucene/AbstractLuceneMetaAlertUpdateDao.java ---
@@ -170,21 +169,51 @@ protected Document buildCreateDocument(Iterable<Document> alerts, List<String> g
return updates;
}
+ /**
+ * Adds alerts to a metaalert, based on a list of GetRequests provided for retrieval.
+ * @param metaAlertGuid The GUID of the metaalert to be given new children.
+ * @param alertRequests GetRequests for the appropriate alerts to add.
+ * @return The updated metaalert with alerts added.
+ */
+ @Override
+ public Document addAlertsToMetaAlert(String metaAlertGuid, List<GetRequest> alertRequests)
+ throws IOException {
+
+ Document metaAlert = retrieveLatestDao
--- End diff --
Don't we need a null check here? What happens if there is no metaAlert?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r222420697
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
Hmm. Interesting. I don't really understand what happens in that else condition then.
The OCD side of me wants to make sure I understand this to ensure there is not a pre-existing bug. But at the same time I can't fault you for this. All you did was refactor this code out into a method.
It seems that a `DocumentContainer` should either have an exception (`dc.getException().isPresent()`) or it should have a document (`dc.getDocument.isPresent()`). We would hit the else when neither of those are the case. So under what conditions would a `DocumentContainer` have neither of those?
I guess a `null` Document or null Throwable is added to the `DocumentContainer`? I hope this is what we expect to happen.
```
if(dc.getException().isPresent()) {
..
} else if(dc.getDocument.isPresent()) {
..
} else {
// when does this occur? what do we expect to happen?
}
```
---
[GitHub] metron issue #1190: METRON-1771: Update REST endpoints to support eventually...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1190
The latest commit updates the various places where looking up a document that doesn't exist returns null. Now an IOException is thrown with a helpful message and guid of the missing object. I added tests to cover these cases. I also added changes that will throw an exception when alerts to be add/removed don't exist. Previously that wasn't covered and the operation would succeed even though the missing alert wasn't added/removed.
For the parallel stream issue, I made it consistent with the rest of the class. It now uses the same approach as the getLatest method and uses the DocumentContainer class. As part of this I moved common code to a separate method.
Let me know what you think about these changes.
---
[GitHub] metron issue #1190: METRON-1771: Update REST endpoints to support eventually...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/1190
+1 Looks great. Works great. I ran this up in Full Dev and tested manually through the Alerts UI. Looking forward to the follow-on UI changes.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217462174
--- Diff: metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.elasticsearch.dao;
+
+import org.apache.metron.indexing.dao.AccessConfig;
+import org.apache.metron.indexing.dao.UpdateDaoTest;
+import org.apache.metron.indexing.dao.update.UpdateDao;
+import org.elasticsearch.client.transport.TransportClient;
+import org.junit.Before;
+
+import static org.mockito.Mockito.mock;
+
+public class ElasticsearchUpdateDaoTest extends UpdateDaoTest {
--- End diff --
No problem
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217438881
--- Diff: metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.elasticsearch.dao;
+
+import org.apache.metron.indexing.dao.AccessConfig;
+import org.apache.metron.indexing.dao.UpdateDaoTest;
+import org.apache.metron.indexing.dao.update.UpdateDao;
+import org.elasticsearch.client.transport.TransportClient;
+import org.junit.Before;
+
+import static org.mockito.Mockito.mock;
+
+public class ElasticsearchUpdateDaoTest extends UpdateDaoTest {
--- End diff --
What is this test for? There seems to be no actual tests. Maybe you forgot to push a commit?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/1190
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217461869
--- Diff: metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.elasticsearch.dao;
+
+import org.apache.metron.indexing.dao.AccessConfig;
+import org.apache.metron.indexing.dao.UpdateDaoTest;
+import org.apache.metron.indexing.dao.update.UpdateDao;
+import org.elasticsearch.client.transport.TransportClient;
+import org.junit.Before;
+
+import static org.mockito.Mockito.mock;
+
+public class ElasticsearchUpdateDaoTest extends UpdateDaoTest {
--- End diff --
Ah, gotcha. Could you add that to the javadoc?
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r216090892
--- Diff: metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/InMemoryMetaAlertRetrieveLatestDao.java ---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.indexing;
+
+import org.apache.metron.indexing.dao.IndexDao;
+import org.apache.metron.indexing.dao.metaalert.MetaAlertRetrieveLatestDao;
+import org.apache.metron.indexing.dao.search.GetRequest;
+import org.apache.metron.indexing.dao.update.Document;
+
+import java.io.IOException;
+import java.util.List;
+
+public class InMemoryMetaAlertRetrieveLatestDao implements MetaAlertRetrieveLatestDao {
--- End diff --
This class doesn't do much but is needed to compose an InMemoryMetaAlertUpdateDao.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1190#discussion_r217459943
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/MultiIndexDao.java ---
@@ -282,4 +276,27 @@ public Document getLatest(final String guid, String sensorType) throws IOExcepti
public List<IndexDao> getIndices() {
return indices;
}
+
+ private Document getDocument(List<DocumentContainer> documentContainers) throws IOException {
+ Document ret = null;
+ List<String> error = new ArrayList<>();
+ for(DocumentContainer dc : documentContainers) {
+ if(dc.getException().isPresent()) {
+ Throwable e = dc.getException().get();
+ error.add(e.getMessage() + "\n" + ExceptionUtils.getStackTrace(e));
+ }
+ else {
+ if(dc.getDocument().isPresent()) {
+ Document d = dc.getDocument().get();
+ if(ret == null || ret.getTimestamp() < d.getTimestamp()) {
+ ret = d;
+ }
+ }
+ }
+ }
--- End diff --
I like it. Will make that change.
---