-
Notifications
You must be signed in to change notification settings - Fork 512
METRON-2034: REST endpoint for getting all parser topology status should return group name #1396
base: feature/METRON-1856-parser-aggregation
Are you sure you want to change the base?
METRON-2034: REST endpoint for getting all parser topology status should return group name #1396
Conversation
@merrimanr I've tested it with full dev and now the UI is able to display if a group of sensors is running or not properly. So this is fine but there's another issue. The reason why it has become a blocker for the latest release candidate is that And the topology info for the three of them coming back from the So currently, the name of the topology item regarding the three aforementioned sensors is "bro__snort__yaf" but there is no group with the same name so the UI can't figure it's a group and the three sensors belong to it. |
You're correct @ruffle1986. The group should already be added when full dev spins up rather than having to add it in a separate step. I am testing a change that should fix this and will push it out once that's done. |
The latest commit should setup the parser group in full dev automatically. |
@@ -6,5 +6,16 @@ | |||
"update.hbase.table": "metron_update", | |||
"update.hbase.cf": "t", | |||
"es.client.settings": { | |||
} | |||
}, | |||
"parser.groups": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this option in global config? I don't recall seeing this before, and it's not mentioned anywhere from what I can tell, e.g. https://github.com/apache/metron/tree/master/metron-platform/metron-common#global-configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parser groups are stored in the global config. There are REST endpoints for managing parser groups, a user would not directly edit this in the global config. I can add it to that README but I'm not sure it's useful to a user. I'm happy to add more documentation wherever it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would we store them there and not in zookeeper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would we store them there and not in zookeeper?
@ottobackwards not sure I completely understand the Q here - global.json is stored in ZK. Is your question why this is stored in global.json?
Part of the reason I asked about the docs is because I don't have a good sense of the implications to storing this here. This is the original PR - #1346. @merrimanr what happens when I start a parser group from the CLI, will global.json be out of sync for the UI then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no change to starting a parser group from the CLI. You would still pass them in as a comma-separate list of parsers if you wanted to group parsers together. Same goes for Ambari. This setting is only used if parser topologies are managed in the Management UI. The REST service handles converting parser groups to the correct CLI arguments and Storm job names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parser groups are stored in the global config. There are REST endpoints for managing parser groups, a user would not directly edit this in the global config. I can add it to that README but I'm not sure it's useful to a user. I'm happy to add more documentation wherever it makes sense.
Can you add documentation for this feature? It looks like it was missed in the original PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What documentation do you want me to add? Do you want a dedicated section on parser groups somewhere? Just add the setting to the global config section in the common README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to explain what it is if I've never seen this config option before, but am familiar with global config. From what you said earlier, this is purely a sharing mechanism, not something a user should ever modify. Some of this may be defined implicitly from the parser aggregation feature, mostly concerned with how this additional feature functions in concert with it.
This is just off the top of my head skimming through, so I imagine you'll have a lot more to add to it as the implementer. Some of this may already be documented out there somewhere, just want to make sure we put a bow on it.
User-level concerns
- If a user does modify it manually, whether intentionally or by accident, what will happen?
- What processes manage this config? Will Ambari or the config UI manage or overwrite these values?
- If a user adds a group manually, will the values remain, or get overwritten/removed? (e.g. what's the interaction with our global config JSON Patching?)
- How is a group different from a parser aggregation as defined in the rest of our documentation?
Developer concerns
- Same as above, plus
- What is the unique ID for a group, is there one?
- What if I group the same sensors in multiple groups?
- What are each of the fields and how are they used? i.e. If I go modify this feature in the UI, what is my API and what are the guarantees it provides?
- What fields are required vs optional? Related to the "guarantees" in 4. Important for null checks, type checks, etc. etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time trying to document this and I realized the documentation doesn't make much sense because the UI component hasn't been done yet. I find myself guessing how it is going to work because I'm not 100% clear on how the UI will function and I don't have anything to reference because that work is still being submitted. Almost all of the requests in your list involve the UI in some way. Parser group and parser aggregation are synonymous so that's something else we'll want to fix.
I think I may put this on hold until the UI part is further along unless you would accept that work as a follow on Jira that blocks the acceptance of this feature branch. The benefit of committing this now is it will resolve a bug that was found earlier.
@merrimanr now that https://github.com/apache/metron/tree/feature/METRON-1856-parser-aggregation is under way, do we have enough detail to bridge the gap between what the UI needs and the backend piece, in particular the documentation? I'm not completely sure if global config is the best place for it or not, since this only really seems to be leverage by REST application (ie, it's not used globally), but I don't have a particularly excellent alternative to recommend currently. |
We are not at a point in the review where we can spin up or test anything. Agreed, documentation belongs in the REST README. |
Let's hold off on this item until the end, or whenever we can reasonably start doing some validation with the actual UI components in this feature branch. It sounds like this may have come from some offline collaboration and testing, so we should make sure that this accomplishes the desired results now that the aggregation feature is going through review. |
I filed this as a bug back in March based on feedback from the contributors working on the parser aggregation feature branch. This fix is required for that feature to work correctly, which is why I think we should move documentation to a follow on Jira. |
Can you link to what you're describing? I'm not seeing much on the ticket - https://issues.apache.org/jira/browse/METRON-2034 |
Are you saying you want a more detailed description the bug as it relates to the UI? This might be easier to demonstrate when the UI part is further along. |
I'm not seeing where the "based on feedback from the contributors working on the parser aggregation feature branch" interaction happened. Was that a private message/interaction? All I'm saying is that if there was a comment on another PR, or a discuss thread, or a convo in Slack - just add it here for context. |
It was a Slack conversation. @ruffle1986 was testing and noticed group names were not working for the endpoint that gets status for all topologies (vs the endpoint that gets status for a single topology). He brought it to my attention and I created this fix as a result. |
I'm not sure I see the point of splitting up a documentation task here - seems like extra overhead with an unclear benefit, but if that's your preference, you're welcome to add a sub-task to the master feature branch Jira that would need to be checked off before the FB is completed. |
Contributor Comments
This PR fixes a bug in the REST application that fails to convert a parser topology id to a group name when calling the
http://node1:8082/api/v1/storm
endpoint. TheStormStatusServiceImplTest
was also updated to included this case.Testing
This has been tested in full dev:
The
bro__snort__yaf
Storm topology should be returned with the name set togroup1
:Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html
:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.