Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-2034: REST endpoint for getting all parser topology status should return group name #1396

Open
wants to merge 2 commits into
base: feature/METRON-1856-parser-aggregation
Choose a base branch
from
Open

METRON-2034: REST endpoint for getting all parser topology status should return group name #1396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e83ad75
initial commit
merrimanr Apr 29, 2019
bc09a05
added parser group to initial global config
merrimanr Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions ...metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import org.apache.metron.common.configuration.SensorParserGroup;
Expand Down Expand Up @@ -83,9 +84,14 @@ public TopologyStatus getTopologyStatus(String name) {
public List<TopologyStatus> getAllTopologyStatus() {
List<TopologyStatus> topologyStatus = new ArrayList<>();
for (TopologyStatus topology : getTopologySummary().getTopologies()) {
topologyStatus.add(restTemplate
.getForObject(getStormUiProperty() + TOPOLOGY_URL + "/" + topology.getId(),
TopologyStatus.class));
TopologyStatus status = restTemplate
.getForObject(getStormUiProperty() + TOPOLOGY_URL + "/" + topology.getId(),
TopologyStatus.class);
if (status != null) {
Optional<String> groupName = topologyNameToGroupName(status.getName());
groupName.ifPresent(status::setName);
topologyStatus.add(status);
}
}
return topologyStatus;
}
Expand Down Expand Up @@ -149,15 +155,10 @@ protected String getTopologyId(String name) {
String topologyName = topology.getName();

// check sensor group
if (topologyName.contains(ParserTopologyCLI.STORM_JOB_SEPARATOR)) {
Set<String> sensors = new HashSet<>(Arrays.asList(topologyName.split(ParserTopologyCLI.STORM_JOB_SEPARATOR)));
SensorParserGroup group = sensorParserGroupService.findOne(name);
if (group == null) {
break;
} else if (sensors.equals(group.getSensors())){
id = topology.getId();
break;
}
Optional<String> groupName = topologyNameToGroupName(topologyName);
if (groupName.isPresent() && groupName.get().equals(name)) {
id = topology.getId();
break;
}

if (topologyName.equals(name)) {
Expand All @@ -167,4 +168,18 @@ protected String getTopologyId(String name) {
}
return id;
}

public Optional<String> topologyNameToGroupName(String topologyName) {
Optional<String> groupName = Optional.empty();
if (topologyName.contains(ParserTopologyCLI.STORM_JOB_SEPARATOR)) {
Set<String> sensors = new HashSet<>(Arrays.asList(topologyName.split(ParserTopologyCLI.STORM_JOB_SEPARATOR)));
for (SensorParserGroup group: sensorParserGroupService.getAll().values()) {
if (sensors.equals(group.getSensors())) {
groupName = Optional.of(group.getName());
break;
}
}
}
return groupName;
}
}
48 changes: 34 additions & 14 deletions ...on-rest/src/test/java/org/apache/metron/rest/service/impl/StormStatusServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ public void getTopologyStatusByGroupShouldReturnTopologyStatus() throws Exceptio
add("bro");
add("snort");
}});
when(sensorParserGroupService.findOne("group")).thenReturn(group);
when(sensorParserGroupService.getAll()).thenReturn(new HashMap() {{
put("group", group);
}});
when(environment.getProperty(STORM_UI_SPRING_PROPERTY)).thenReturn(HTTP_STORM_UI);
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_SUMMARY_URL, TopologySummary.class)).thenReturn(topologySummary);
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_URL + "/bro_snort_id", TopologyStatus.class)).thenReturn(topologyStatus);
Expand All @@ -165,23 +167,41 @@ public void getTopologyStatusByGroupShouldReturnTopologyStatus() throws Exceptio

@Test
public void getAllTopologyStatusShouldReturnAllTopologyStatus() throws Exception {
final TopologyStatus topologyStatus = new TopologyStatus();
topologyStatus.setStatus(TopologyStatusCode.STARTED);
topologyStatus.setName("bro");
topologyStatus.setId("bro_id");
final TopologyStatus broTopologyStatus = new TopologyStatus();
broTopologyStatus.setStatus(TopologyStatusCode.STARTED);
broTopologyStatus.setName("bro");
broTopologyStatus.setId("bro_id");
final TopologyStatus groupTopologyStatus = new TopologyStatus();
groupTopologyStatus.setStatus(TopologyStatusCode.STARTED);
groupTopologyStatus.setName("snort__yaf");
groupTopologyStatus.setId("snort__yaf_id");
final TopologySummary topologySummary = new TopologySummary();
topologySummary.setTopologies(new TopologyStatus[]{topologyStatus});
topologySummary.setTopologies(new TopologyStatus[]{broTopologyStatus, groupTopologyStatus});

SensorParserGroup group = new SensorParserGroup();
group.setName("group");
group.setSensors(new HashSet<String>() {{
add("snort");
add("yaf");
}});
when(sensorParserGroupService.getAll()).thenReturn(new HashMap() {{
put("group", group);
}});
when(environment.getProperty(STORM_UI_SPRING_PROPERTY)).thenReturn(HTTP_STORM_UI);
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_SUMMARY_URL, TopologySummary.class)).thenReturn(topologySummary);
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_URL + "/bro_id", TopologyStatus.class)).thenReturn(topologyStatus);

TopologyStatus expected = new TopologyStatus();
expected.setStatus(TopologyStatusCode.STARTED);
expected.setName("bro");
expected.setId("bro_id");

assertEquals(new ArrayList() {{ add(expected); }}, stormStatusService.getAllTopologyStatus());
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_URL + "/bro_id", TopologyStatus.class)).thenReturn(broTopologyStatus);
when(restTemplate.getForObject(HTTP_STORM_UI + TOPOLOGY_URL + "/snort__yaf_id", TopologyStatus.class)).thenReturn(groupTopologyStatus);

TopologyStatus expectedBro = new TopologyStatus();
expectedBro.setStatus(TopologyStatusCode.STARTED);
expectedBro.setName("bro");
expectedBro.setId("bro_id");
TopologyStatus expectedGroup = new TopologyStatus();
expectedGroup.setStatus(TopologyStatusCode.STARTED);
expectedGroup.setName("group");
expectedGroup.setId("snort__yaf_id");

assertEquals(new ArrayList() {{ add(expectedBro); add(expectedGroup); }}, stormStatusService.getAllTopologyStatus());
}


Expand Down
13 changes: 12 additions & 1 deletion metron-platform/metron-common/src/main/config/zookeeper/global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,16 @@
"update.hbase.table": "metron_update",
"update.hbase.cf": "t",
"es.client.settings": {
}
},
"parser.groups": [
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes no problem.

Copy link
Contributor

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.

Copy link
Contributor Author

@merrimanr merrimanr May 9, 2019 •
edited
Loading

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?

Copy link
Contributor

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

  1. If a user does modify it manually, whether intentionally or by accident, what will happen?
  2. What processes manage this config? Will Ambari or the config UI manage or overwrite these values?
  3. 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?)
  4. How is a group different from a parser aggregation as defined in the rest of our documentation?

Developer concerns

  1. Same as above, plus
  2. What is the unique ID for a group, is there one?
  3. What if I group the same sensors in multiple groups?
  4. 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?
  5. What fields are required vs optional? Related to the "guarantees" in 4. Important for null checks, type checks, etc. etc.

Copy link
Contributor Author

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.

{
"name": "group",
"description": "group",
"sensors": [
"bro",
"snort",
"yaf"
]
}
]
}