METRON-503: Metron REST API #316
Conversation
|
@merrimanr On this one, we need to address a licensing issue. |
… local filesystem. Also fixed some corner cases for sensorParserConfigHistoryService (history doesn't exist, deleting a history that doesn't exist, etc). Added context path to integration tests.
merrimanr
changed the title
|
Just updated this PR to include several changes/improvements:
Documentation for installing the REST service is included in the README. To test in an IDE against a Metron Docker environment (assuming that is running):
I am planning on making this more straightforward with a Maven profile soon. During a review of the licenses we learned that the Hibernate license is not Apache friendly. I am planning to substitute EclipseLink as the JPA implementation in the future. This is why the Hibernate installation is a separate install step. |
There was a problem hiding this comment.
Ok, first VERY quick pass at this. It's a lot of code and expect a few more passes.
I'd like to request kindly that the single +1 rule be abbreviated for this PR and the UI PR. They're really big and I think at least 2 committers should give their blessing before they get in.
As always, thanks for the contribution. A middle tier is very welcome addition. :)
| auth | ||
| .jdbcAuthentication() | ||
| .dataSource(dataSource) | ||
| .withUser("user").password("password").roles("USER").and() |
There was a problem hiding this comment.
Are these hard coded users? Are there instructions on adding new users?
There was a problem hiding this comment.
Instructions have been added to the README.
| public class GrokService { | ||
|
|
||
| public static final String GROK_PATH_SPRING_PROPERTY = "grok.path"; | ||
| public static final String GROK_CLASS_NAME = "org.apache.metron.parsers.GrokParser"; |
There was a problem hiding this comment.
Can we do GrokParser.class.toString() here instead?
There was a problem hiding this comment.
This is done with the assumption you meant "GrokParser.class.getName()".
| if (!isTemporary) { | ||
| hdfsService.write(new Path(grokPath), fullGrokStatement.getBytes()); | ||
| } else { | ||
| FileWriter fileWriter = new FileWriter(new File(grokPath)); |
There was a problem hiding this comment.
What if multiple users are using this simultaneously? Won't these temp files conflict?
There was a problem hiding this comment.
I added a property in the config to set the temporary path and appended a "user" subdirectory.
| add("geo"); | ||
| add("host"); | ||
| add("whois"); | ||
| add("sample"); |
| } | ||
|
|
||
| @Autowired | ||
| private GrokService grokService; |
There was a problem hiding this comment.
Why do we have a GrokService in a more general SensorParserConfigService? Is there something coupled between grok and the general parser?
Half of this class seems grok specific and the rest is generic to BasicParser, so I'm wondering if this could be abstracted differently.
There was a problem hiding this comment.
Most of the code in the GrokService class handles reading and writing Grok statements from/to HDFS. If we merge the PR to keep grok statements in Zookeeper this class can go away.
There was a problem hiding this comment.
The grok service should be replaced with a generic component interface that handles configurations to/from X, with hdfs and zookeeper implementations in general, but this is the wrong pr
|
|
||
| protected String[] getParserStartCommand(String name) { | ||
| String[] command = new String[7]; | ||
| command[0] = environment.getProperty(PARSER_SCRIPT_PATH_SPRING_PROPERTY); |
There was a problem hiding this comment.
Do these services have to be running on a node with the metron scripts? It appears so from this, but I wanted to make sure. If it is, can we make sure we have that spelled out specifically in the README.md
There was a problem hiding this comment.
This has been added to the prerequisites section of the README.
| return stellarFunctionDescriptions; | ||
| } | ||
|
|
||
| private List<String> simpleFunctionNames = Arrays.asList( |
There was a problem hiding this comment.
Can you pull these from the classpath based on the Stellar annotation? I presume "simple" in this case are stellar functions with one argument (that might be a relevant comment in the code actually). You should be able to get the list of stellar functions via reflection and then get the number of params to filter them out.
| 1. Start the UI with this command: | ||
| ``` | ||
| ./bin/start.sh /path/to/application.yml | ||
| ``` |
There was a problem hiding this comment.
Can we get a section laying out and documenting all of the API endpoints for these services, split by service?
There was a problem hiding this comment.
I added annotations to all the Controller methods, much like the Stellar functions. I also created a script that parses the annotations and generates the README from a velocity template. If we decide we like that approach and want to add it to Stellar, we can look at incorporating it into our Maven build process.
- hbase config is now read from Storm classpath instead of hbase jar - removed global config push from kafkazk init script - added license to hbase/bin/init-commands.txt - improved data generator script and updated documentation
|
Just pushed out commits to address these bugs. In regards to this: "Another thing that may be nice is to describe what is all needed for the REST API to run. For example, I believe it needs access to storm cli, some scripts, and some other things." This is included at the top of the metron-rest README in the Prerequisites section. It's changed a couple times so you may have just missed it. If there are other dependencies you think we should call out, let me know. Thanks for the feedback. I think we're almost there. |
|
Removing the org.reflections dependency from the metron-rest pom and inheriting it from metron-common seems to have resolved the "java.lang.NoSuchMethodError" issue with the Stellar endpoints. Changing the metron-common shade plugin to not install the metron-common uber jar by default also solved the issue but that belongs in another Jira. |
|
@merrimanr For some reason I am still getting the following error on the endpoints. Also, the no such method error turned into this as well. How are you running it? I'm running it using java cli. Are you using IntelliJ or something else? |
|
Just merged in the latest from master including the maxmind fix that was breaking the build. I am not able to recreate the NoSuchError exception. Here is what I'm doing to test: When I hit the stellar endpoints, the functions come back without error. How are you testing it? |
|
@merrimanr I'm using quick-dev to do my tests. I built everything from metron root with |
|
Hmm I just tried deploying on quick-dev the same way and was able to hit the stellar endpoints without issue. I will continue testing to see if I can get it to break tomorrow. |
|
@merrimanr I had a chance to try this again. If I build from |
# Conflicts: # dependencies_with_url.csv
|
Finally figured this one out. I wasn't able to reproduce the issue because my maven version was a couple minor releases behind (on 3.3.9 now for what it's worth). Once I got past that the problem was obvious: metron-common was bringing in shaded org.reflections classes directly and regular org.reflections classes transitively which are not compatible because guava is rewritten in the shaded classes, hence the NoSuchMethodError. So for now the fix is to exclude org.reflections from the metron-common dependency and only depend on the shaded version. I had to merge several commits in so I'm still testing for regressions but this issue should be resolved. |
|
@merrimanr Good catch on this. I'll take a look again, but I think it's looking good! |
|
First off, I'd like to apologize for it taking me a little longer to retest this. I went through and tested the issues I saw again. Everything seems to be working. I think this is ready to go. It's a great feature. Thanks for implementing it! Also, since this is a big PR, don't worry about giving me credit on my commits. Especially if it makes your life easier when squashing everything. If that's allowed anyway. +1 |
|
Ok, this is a big one and has been around for a looooong time now in Metron-years. Before we go ahead and pull the trigger on commit, I want to make sure there are no outstanding comments that need to be addressed. |
|
+1 by the way ;) |
|
@merrimanr @jjmeyer0 I think we should make sure JJ gets credit for his contribution on this. We should squash the commits into the minimal possible each named "METRON-503: Metron REST API this closes #316" with the appropriate authorship. |
|
For convenience, I staged the commit in a remote branch: https://github.com/cestella/incubator-metron/tree/METRON-503_sandbox . It'll run in my personal travis account. I'd ask that @merrimanr take it for a test drive. |
|
All integration tests are passing. I was able to start up the application against quick dev and all the endpoints look like they are working. |
This pull request is the first version of the REST API. I intentionally kept it simple by including only one well understood service: CRUD interface for SensorParserConfigs. While I feel it's solid (100% test coverage, integration tests that leverages in-memory component testing infrastructure, no PMD warnings, etc), there are several areas that should be reviewed:
I'm sure there are other issues I'm not thinking of. Once we can come to a consensus on this initial PR, other services can be added that follow the same patterns we establish here.
This can be tested by either running the unit/integration tests or running the service against a vagrant environment. To do the latter:
There is much more to do (deployment, packaging, etc) but this should get us started.