You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by cestella <gi...@git.apache.org> on 2017/12/11 16:13:43 UTC
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
GitHub user cestella opened a pull request:
https://github.com/apache/metron/pull/863
METRON-1347: Indexing Topology should fail tuples without a source.type
## Contributor Comments
If you are sending data into metron indexing without a source.type, which would only happen if you're bypassing our previous topologies, we cannot configure how we write to the indices, so the message should be explicitly failed and reported.
To test this, you can send a message into the indexing topic via kafka console producer that looks like:
```
{
"value" : "foo"
}
```
Ensure that the tuple is failed and an error message indicating that a `source.type` is required is written to the storm UI.
## 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 && 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:
- [x] 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/cestella/incubator-metron METRON-1347
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/863.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 #863
----
commit 78fda5f56562463c7c3dbe3a06a0dbc54e496c83
Author: cstella <ce...@gmail.com>
Date: 2017-12-11T16:11:19Z
METRON-1347: Indexing Topology should fail tuples without a source.type
----
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/863
I would like to hear feedback from @ottobackwards on other required fields but this looks good to me otherwise.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/863
My +1 still stands on this and this PR has been up for quite some time. I will merge by EOD unless there are any additional concerns.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/863
Actually, I don't think `original_string` is required past the parser topology. For instance, profiler messages into enrichment do not have `original_string`.
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r161087829
--- Diff: metron-platform/metron-indexing/README.md ---
@@ -15,6 +15,12 @@ Indices are written in batch and the batch size and batch timeout are specified
[Sensor Indexing Configuration](#sensor-indexing-configuration) via the `batchSize` and `batchTimeout` parameters.
These configs are variable by sensor type.
+## Minimal Assumptions for Message Structure
+
+At minimum, a message should have a `sensor.type` field.
--- End diff --
whoops, yes, typo. corrected.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/863
@ottobackwards created this to track it - https://issues.apache.org/jira/browse/METRON-1524
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156372868
--- Diff: metron-platform/metron-writer/src/main/java/org/apache/metron/writer/bolt/BulkMessageWriterBolt.java ---
@@ -229,17 +239,30 @@ public void execute(Tuple tuple) {
LOG.trace("Writing enrichment message: {}", message);
WriterConfiguration writerConfiguration = configurationTransformation.apply(
new IndexingWriterConfiguration(bulkMessageWriter.getName(), getConfigurations()));
- if(writerConfiguration.isDefault(sensorType)) {
- //want to warn, but not fail the tuple
- collector.reportError(new Exception("WARNING: Default and (likely) unoptimized writer config used for " + bulkMessageWriter.getName() + " writer and sensor " + sensorType));
+ if(sensorType == null) {
--- End diff --
Maybe we should validate all the required fields?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by simonellistonball <gi...@git.apache.org>.
Github user simonellistonball commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156676868
--- Diff: metron-platform/metron-writer/src/main/java/org/apache/metron/writer/bolt/BulkMessageWriterBolt.java ---
@@ -229,17 +239,30 @@ public void execute(Tuple tuple) {
LOG.trace("Writing enrichment message: {}", message);
WriterConfiguration writerConfiguration = configurationTransformation.apply(
new IndexingWriterConfiguration(bulkMessageWriter.getName(), getConfigurations()));
- if(writerConfiguration.isDefault(sensorType)) {
- //want to warn, but not fail the tuple
- collector.reportError(new Exception("WARNING: Default and (likely) unoptimized writer config used for " + bulkMessageWriter.getName() + " writer and sensor " + sensorType));
+ if(sensorType == null) {
--- End diff --
Strictly speaking that's true, but by convention original_string should be required. There is a broader topic about what should be required, but that certainly doesn't belong in a comment on a PR.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/863
The parser topology PRODUCES the original_string. By that very action, and it's requirement, it can only be used and required by downstream usages either from rest ( post indexing ) or during enrichment. It seems inconsistent to validate without including that, if what you want to do is validate the entire set of required fields. If you only want to handle what is required at a given step, but not what is required 'overall' then this is OK, but something doesn't seem right here conceptually.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/863
It seems this PR is meant to address the global case, since the writer is general purpose. Per @cestella's comments, I don't know that we can include original_string. I do think there's value in minimally ensuring the timestamp and source.type fields are validated in the near term. It seems that addressing a broader validation strategy might be in order, but maybe we could handle that separately as a compromise? Thoughts @ottobackwards?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156372312
--- Diff: metron-platform/metron-indexing/README.md ---
@@ -15,6 +15,12 @@ Indices are written in batch and the batch size and batch timeout are specified
[Sensor Indexing Configuration](#sensor-indexing-configuration) via the `batchSize` and `batchTimeout` parameters.
These configs are variable by sensor type.
--- End diff --
Do we care, here in particular, that we have different required fields listed? Should this not be cumulative and include the required fields coming out of the parsers?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156675356
--- Diff: metron-platform/metron-writer/src/main/java/org/apache/metron/writer/bolt/BulkMessageWriterBolt.java ---
@@ -229,17 +239,30 @@ public void execute(Tuple tuple) {
LOG.trace("Writing enrichment message: {}", message);
WriterConfiguration writerConfiguration = configurationTransformation.apply(
new IndexingWriterConfiguration(bulkMessageWriter.getName(), getConfigurations()));
- if(writerConfiguration.isDefault(sensorType)) {
- //want to warn, but not fail the tuple
- collector.reportError(new Exception("WARNING: Default and (likely) unoptimized writer config used for " + bulkMessageWriter.getName() + " writer and sensor " + sensorType));
+ if(sensorType == null) {
--- End diff --
Sure thing. Really the only two required are `timestamp` and `source.type`. Did I miss any?
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/863
The minimum required fields, as far as I can see right now are source.type, original_string and timestamp. Given the use case for this is something that has skipped the parser topology, we should validate those.
If we think the same can be done for indexing, then we should use the same classes/technique there.
Again, this is based on the presented use case
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/863
@mmiklavc @cestella @ottobackwards Is this good to be merged in?
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/863
Are all questions on required fields resolved per @cestella's comments/clarification?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156676155
--- Diff: metron-platform/metron-indexing/README.md ---
@@ -15,6 +15,12 @@ Indices are written in batch and the batch size and batch timeout are specified
[Sensor Indexing Configuration](#sensor-indexing-configuration) via the `batchSize` and `batchTimeout` parameters.
These configs are variable by sensor type.
--- End diff --
So, strictly speaking messages really only require `source.type` (which I typo'd) and `timestamp` (which I should add). I'll fix that, but did I miss anything?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r160250862
--- Diff: metron-platform/metron-indexing/README.md ---
@@ -15,6 +15,12 @@ Indices are written in batch and the batch size and batch timeout are specified
[Sensor Indexing Configuration](#sensor-indexing-configuration) via the `batchSize` and `batchTimeout` parameters.
These configs are variable by sensor type.
+## Minimal Assumptions for Message Structure
+
+At minimum, a message should have a `sensor.type` field.
--- End diff --
sensor.type - did you mean source.type?
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/863
I think this is a mess that we will need to sort out, but I won't hold this up for that.
I'm +0 I guess.
---
[GitHub] metron issue #863: METRON-1347: Indexing Topology should fail tuples without...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/863
What I think would be better would be to have a readme about the required fields and refer to that document from these other documents
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/863#discussion_r156674159
--- Diff: metron-platform/metron-writer/src/main/java/org/apache/metron/writer/bolt/BulkMessageWriterBolt.java ---
@@ -229,17 +239,30 @@ public void execute(Tuple tuple) {
LOG.trace("Writing enrichment message: {}", message);
WriterConfiguration writerConfiguration = configurationTransformation.apply(
new IndexingWriterConfiguration(bulkMessageWriter.getName(), getConfigurations()));
- if(writerConfiguration.isDefault(sensorType)) {
- //want to warn, but not fail the tuple
- collector.reportError(new Exception("WARNING: Default and (likely) unoptimized writer config used for " + bulkMessageWriter.getName() + " writer and sensor " + sensorType));
+ if(sensorType == null) {
--- End diff --
@ottobackwards which fields should we validate here?
---
[GitHub] metron pull request #863: METRON-1347: Indexing Topology should fail tuples ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/863
---