[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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
documentContainers) throws IOException {
Document latestDocument = null;
List 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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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...
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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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 pull request #1190: METRON-1771: Update REST endpoints to support eve...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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...
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...
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...
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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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.
---
[GitHub] metron pull request #1190: METRON-1771: Update REST endpoints to support eve...
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...
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...
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 getIndices() {
return indices;
}
+
+ private Document getDocument(List documentContainers)
throws IOException {
+Document ret = null;
+List 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...
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> 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 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 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...
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...
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> 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...
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 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...
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> 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...
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...
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 alerts, List 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 alertRequests)
+ throws IOException {
+
+Document metaAlert = retrieveLatestDao
+.getLatest(metaAlertGuid, MetaAlertConstants.METAALERT_TYPE);
+if (MetaAlertStatus.ACTIVE.getStatusString()
+
.equals(metaAlert.getDocument().get(MetaAlertConstants.STATUS_FIELD))) {
+ Iterable alerts =
retrieveLatestDao.getAllLatest(alertRequests);
+ Map> 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 alertRequests)
- throws IOException {
+ public Document removeAlertsFromMetaAlert(String metaAlertGuid,
List 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 pull request #1190: METRON-1771: Update REST endpoints to support eve...
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> commentMap = (List>)
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...
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...
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...
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...
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...
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 alerts, List 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 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...
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...
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
getRequests)
throw new OriginalNotFoundException("Count not find guids after " +
MAX_RETRIES + "tries");
}
+ @SuppressWarnings("unchecked")
+ protected void assertEquals(Map expected, Map 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...
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...
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 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 threatScores = new ArrayList<>();
-Collection 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 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...
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...
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 alerts, List 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 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...
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...
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
