You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by ottobackwards <gi...@git.apache.org> on 2017/06/03 16:54:14 UTC
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
GitHub user ottobackwards opened a pull request:
https://github.com/apache/metron/pull/607
METRON-982 add new rest api for storm supervisor status
This pr adds a new rest endpoint for retrieving storm supervisor summary information from the "/api/v1/supervisor/summary" storm ui rest endpoint.
This information contains the currently configured slots for each supervisor, and how many are used.
This is in support of METRON-981, which will be able to use this endpoint to enable errors and warnings based on the lack of available slots.
###TESTING
In full_dev environment execute the new endpoint from swagger, you should see the supervisor information from storm
/supervisors off of the storm controller
### 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:
- [NA ] 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 incubating-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?
- [ NA ] 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)?
- [ x ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ottobackwards/metron METRON-982
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/607.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 #607
----
commit 1593a0fe0fe03ba8f51cd1f711854bdae6a234ab
Author: Otto Fowler <ot...@gmail.com>
Date: 2017-06-03T15:22:01Z
add new rest api for supervisor status
commit 5337d8f294bbe8df739070a49187fc3e4dc711b7
Author: Otto Fowler <ot...@gmail.com>
Date: 2017-06-03T16:22:31Z
add asf headers
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r122700117
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
Should have pointed this out the other day, but that PR is in now, so this probably should be:
```
return restTemplate.getForObject(getStormUiProperty() + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
```
Ping me if there's any issues and I'm happy to do more than just glance at it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/607
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129692618
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
This also needs to be "uptime"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #607: METRON-982 add new rest api for storm supervisor status
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/607
Hey @ottobackwards, list has been on my todo list for a while. I'll spin it up today or tomorrow. Sorry for the delay.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129923353
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
I don't understand, the API spec says it is upTime
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129927345
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
Which API spec are you referring to? This [page](http://storm.apache.org/releases/1.0.3/STORM-UI-REST-API.html) has it as "uptime".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #607: METRON-982 add new rest api for storm supervisor status
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/607
Bump - anyone have any feedback?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129946569
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129929213
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
ok, i don't know how that got formatted to upTime, I have a comment in there that I copied out of that page to go by.....
The getter and setter names can stay though? getUpTime() etc?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #607: METRON-982 add new rest api for storm supervisor status
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/607
I spun this up in full dev and it worked other than "upTime" being null. Pending a few minor suggestions and the upTime fix I'm +1. Nice job.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by simonellistonball <gi...@git.apache.org>.
Github user simonellistonball commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r120112633
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
see https://issues.apache.org/jira/browse/METRON-906
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r120117733
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
I was just following the convention of the service as I found it. I think that is a separate issue as @simonellistonball points out
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #607: METRON-982 add new rest api for storm supervisor status
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/607
@merrimanr we all set?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129930004
--- Diff: metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java ---
@@ -182,6 +185,16 @@ public void test() throws Exception {
.andExpect(jsonPath("$.status").value("SUCCESS"))
.andExpect(jsonPath("$.message").value(TopologyStatusCode.STARTED.name()));
+ this.mockMvc.perform(get(stormUrl + "/supervisors").with(httpBasic(user,password)))
+ .andExpect(status().isOk())
+ .andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
+ .andExpect(jsonPath("$.supervisors[0]").exists())
+ .andExpect(jsonPath("$.supervisors[0].id").exists())
+ .andExpect(jsonPath("$.supervisors[0].host").exists())
+ .andExpect(jsonPath("$.supervisors[0].upTime").exists())
--- End diff --
I changed the getter/setter methods in my test. You can try leaving them the same but I know "getUptime()" works.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129690884
--- Diff: metron-interface/metron-rest-client/src/main/java/org/apache/metron/rest/model/SupervisorStatus.java ---
@@ -0,0 +1,127 @@
+/**
+ * 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.rest.model;
+
+public class SupervisorStatus {
+ /*
+ /api/v1/supervisor/summary (GET)
+ returns all supervisors summary
+
+ Response Fields:
+
+ Field Value Description
+ id String Supervisor's id
+ host String Supervisor's host name
+ upTime String Shows how long the supervisor is running
+ slotsTotal Integer Total number of available worker slots for this supervisor
+ slotsUsed Integer Number of worker slots used on this supervisor
+ Sample Response:
+ json { "supervisors": [ { "id": "0b879808-2a26-442b-8f7d-23101e0c3696", "host": "10.11.1.7", "upTime": "5m 58s", "slotsTotal": 4, "slotsUsed": 3 } ] }
+
+ */
+ private String id;
+ private String host;
+ private String upTime;
--- End diff --
This needs to be "uptime" instead of "upTime" or jackson doesn't map it correctly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129691439
--- Diff: metron-interface/metron-rest-client/src/main/java/org/apache/metron/rest/model/SupervisorStatus.java ---
@@ -0,0 +1,127 @@
+/**
+ * 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.rest.model;
+
+public class SupervisorStatus {
+ /*
+ /api/v1/supervisor/summary (GET)
+ returns all supervisors summary
+
+ Response Fields:
+
+ Field Value Description
+ id String Supervisor's id
+ host String Supervisor's host name
+ upTime String Shows how long the supervisor is running
+ slotsTotal Integer Total number of available worker slots for this supervisor
+ slotsUsed Integer Number of worker slots used on this supervisor
+ Sample Response:
+ json { "supervisors": [ { "id": "0b879808-2a26-442b-8f7d-23101e0c3696", "host": "10.11.1.7", "upTime": "5m 58s", "slotsTotal": 4, "slotsUsed": 3 } ] }
+
+ */
+ private String id;
+ private String host;
+ private String upTime;
+ private int slotsTotal;
+ private int slotsUsed;
+
+ public SupervisorStatus(){}
+ public SupervisorStatus(String id, String host, String upTime, int slotsTotal, int slotsUsed) {
+ this.id = id;
+ this.host = host;
+ this.upTime = upTime;
+ this.slotsTotal = slotsTotal;
+ this.slotsUsed = slotsUsed;
+ }
+
+ public String getId() {
+ return id;
+ }
+
+ public void setId(String id) {
+ this.id = id;
+ }
+
+ public String getHost() {
+ return host;
+ }
+
+ public void setHost(String host) {
+ this.host = host;
+ }
+
+ public String getUpTime() {
+ return upTime;
+ }
+
+ public void setUpTime(String upTime) {
+ this.upTime = upTime;
+ }
+
+ public int getSlotsTotal() {
+ return slotsTotal;
+ }
+
+ public void setSlotsTotal(int slotsTotal) {
+ this.slotsTotal = slotsTotal;
+ }
+
+ public int getSlotsUsed() {
+ return slotsUsed;
+ }
+
+ public void setSlotsUsed(int slotsUsed) {
+ this.slotsUsed = slotsUsed;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ SupervisorStatus that = (SupervisorStatus) o;
+
+ if (getSlotsTotal() != that.getSlotsTotal()) {
+ return false;
+ }
+ if (getSlotsUsed() != that.getSlotsUsed()) {
+ return false;
+ }
+ if (!getId().equals(that.getId())) {
+ return false;
+ }
+ if (!getHost().equals(that.getHost())) {
+ return false;
+ }
+ return getUpTime().equals(that.getUpTime());
+ }
+
+ @Override
+ public int hashCode() {
+ int result = id != null? id.hashCode() : 0;
+ result = 31 * result + host != null? host.hashCode() : 0;
--- End diff --
I get a warning in Intellij. Should there be parenthesis around `host != null? host.hashCode() : 0`, similar to other classes in metron-rest-client?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129718460
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -17,10 +17,7 @@
*/
package org.apache.metron.rest.service.impl;
-import org.apache.metron.rest.model.TopologyResponse;
-import org.apache.metron.rest.model.TopologyStatus;
-import org.apache.metron.rest.model.TopologyStatusCode;
-import org.apache.metron.rest.model.TopologySummary;
+import org.apache.metron.rest.model.*;
--- End diff --
@merrimanr , heh, you made me go look this up!
As far as I can see, our coding standards, both [here](https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines#DevelopmentGuidelines-2.2CodeStyle) and [here](http://www.oracle.com/technetwork/java/codeconvtoc-136057.html) are silent on this topic.
There is a good discussion of the pros and cons [here](https://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad). Despite the latter being a fairly old article, google did not show me any more recent definitive policy statements.
I must admit that I think the "wildcard imports are evil" side have slightly more weight in the argument, yet I personally find long lists of explicit imports annoying and appreciate IntelliJ's default settings (more than 5 imports of the same package collapses to wildcard). Can you provide cites that support your assertion regarding "most coding standards"?
If you are so moved, you might bring this up on the mailing list, perhaps providing the citations above, and propose a vote to modify the Coding Style.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129904369
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -17,10 +17,7 @@
*/
package org.apache.metron.rest.service.impl;
-import org.apache.metron.rest.model.TopologyResponse;
-import org.apache.metron.rest.model.TopologyStatus;
-import org.apache.metron.rest.model.TopologyStatusCode;
-import org.apache.metron.rest.model.TopologySummary;
+import org.apache.metron.rest.model.*;
--- End diff --
@merrimanr @mattf-horton We did not have an official vote. Given the widespread acceptance of it, since it's not a dev guildelines and not a bylaw thing, my impression was we didn't need it. Having said that, if it needs or should have an actual vote, I'm definitely okay with it and it should be done before https://github.com/apache/metron/pull/577 goes in.
However, 577 is still outstanding pending additional feedback I requested. I was inclined to not update the wiki, because we hadn't built out and validated the tooling at the time. I personally lean towards updating the wiki once we're good with that PR going in.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron issue #607: METRON-982 add new rest api for storm supervisor status
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/607
Just tested this on full dev and it worked great. Thanks @ottobackwards. +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129920967
--- Diff: metron-interface/metron-rest-client/src/main/java/org/apache/metron/rest/model/SupervisorStatus.java ---
@@ -0,0 +1,127 @@
+/**
+ * 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.rest.model;
+
+public class SupervisorStatus {
+ /*
+ /api/v1/supervisor/summary (GET)
+ returns all supervisors summary
+
+ Response Fields:
+
+ Field Value Description
+ id String Supervisor's id
+ host String Supervisor's host name
+ upTime String Shows how long the supervisor is running
+ slotsTotal Integer Total number of available worker slots for this supervisor
+ slotsUsed Integer Number of worker slots used on this supervisor
+ Sample Response:
+ json { "supervisors": [ { "id": "0b879808-2a26-442b-8f7d-23101e0c3696", "host": "10.11.1.7", "upTime": "5m 58s", "slotsTotal": 4, "slotsUsed": 3 } ] }
+
+ */
+ private String id;
+ private String host;
+ private String upTime;
--- End diff --
Where would I see that? Am I missing a test?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129897123
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -17,10 +17,7 @@
*/
package org.apache.metron.rest.service.impl;
-import org.apache.metron.rest.model.TopologyResponse;
-import org.apache.metron.rest.model.TopologyStatus;
-import org.apache.metron.rest.model.TopologyStatusCode;
-import org.apache.metron.rest.model.TopologySummary;
+import org.apache.metron.rest.model.*;
--- End diff --
Thanks for checking me on this @mattf-horton. I was under the impression we already voted to switch our coding standard to Google's based on the [Code Style](http://mail-archives.apache.org/mod_mbox/metron-dev/201705.mbox/%3CCADu=U3A23q-aehhim6R0AceZapAJ9QJvG8hnurhR2hkDtvXNLw@mail.gmail.com%3E) discussion on the dev list back in the beginning of May. There is also a pending [PR](https://github.com/apache/metron/pull/577) that sets up Checkstyle with the Google Code Style.
Was this not an official vote or was the wiki just not updated? If we did switch, [Google Coding Style](https://google.github.io/styleguide/javaguide.html#s3.3-import-statements) does not allow wildcard imports.
This is probably one of those topics with no clear correct answer, similar to spaces vs tabs. I personally am not a fan of wildcard statements because I prefer to see exactly what is being imported.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129926344
--- Diff: metron-interface/metron-rest-client/src/main/java/org/apache/metron/rest/model/SupervisorStatus.java ---
@@ -0,0 +1,127 @@
+/**
+ * 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.rest.model;
+
+public class SupervisorStatus {
+ /*
+ /api/v1/supervisor/summary (GET)
+ returns all supervisors summary
+
+ Response Fields:
+
+ Field Value Description
+ id String Supervisor's id
+ host String Supervisor's host name
+ upTime String Shows how long the supervisor is running
+ slotsTotal Integer Total number of available worker slots for this supervisor
+ slotsUsed Integer Number of worker slots used on this supervisor
+ Sample Response:
+ json { "supervisors": [ { "id": "0b879808-2a26-442b-8f7d-23101e0c3696", "host": "10.11.1.7", "upTime": "5m 58s", "slotsTotal": 4, "slotsUsed": 3 } ] }
+
+ */
+ private String id;
+ private String host;
+ private String upTime;
--- End diff --
When I tested this in full dev with Swagger, the upTime field in the response was null. Here's an example response when I hit http://node1:8744/api/v1/supervisor/summary:
```
{"supervisors":[{"totalMem":3072.0,"host":"node1","id":"70bd7170-160f-4215-8f18-d9088993fb47","uptime":"47m 20s","totalCpu":400.0,"usedCpu":0.0,"logLink":"http:\/\/node1:8000\/daemonlog?file=supervisor.log","usedMem":3328.0,"slotsUsed":4,"version":"1.0.1.2.5.6.0-40","slotsTotal":4,"uptimeSeconds":2840}],"schedulerDisplayResource":false,"logviewerPort":8000}
```
Notice the field is actually "uptime". I changed the upTime variable name and getter/setting methods and that fixed it.
The StormControllerIntegrationTest uses a mock Storm REST API so you're not going to catch it there. You would only catch this by hitting a real Storm REST API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r122703811
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
Thanks! I'll hook that up
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129920992
--- Diff: metron-interface/metron-rest-client/src/main/java/org/apache/metron/rest/model/SupervisorStatus.java ---
@@ -0,0 +1,127 @@
+/**
+ * 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.rest.model;
+
+public class SupervisorStatus {
+ /*
+ /api/v1/supervisor/summary (GET)
+ returns all supervisors summary
+
+ Response Fields:
+
+ Field Value Description
+ id String Supervisor's id
+ host String Supervisor's host name
+ upTime String Shows how long the supervisor is running
+ slotsTotal Integer Total number of available worker slots for this supervisor
+ slotsUsed Integer Number of worker slots used on this supervisor
+ Sample Response:
+ json { "supervisors": [ { "id": "0b879808-2a26-442b-8f7d-23101e0c3696", "host": "10.11.1.7", "upTime": "5m 58s", "slotsTotal": 4, "slotsUsed": 3 } ] }
+
+ */
+ private String id;
+ private String host;
+ private String upTime;
+ private int slotsTotal;
+ private int slotsUsed;
+
+ public SupervisorStatus(){}
+ public SupervisorStatus(String id, String host, String upTime, int slotsTotal, int slotsUsed) {
+ this.id = id;
+ this.host = host;
+ this.upTime = upTime;
+ this.slotsTotal = slotsTotal;
+ this.slotsUsed = slotsUsed;
+ }
+
+ public String getId() {
+ return id;
+ }
+
+ public void setId(String id) {
+ this.id = id;
+ }
+
+ public String getHost() {
+ return host;
+ }
+
+ public void setHost(String host) {
+ this.host = host;
+ }
+
+ public String getUpTime() {
+ return upTime;
+ }
+
+ public void setUpTime(String upTime) {
+ this.upTime = upTime;
+ }
+
+ public int getSlotsTotal() {
+ return slotsTotal;
+ }
+
+ public void setSlotsTotal(int slotsTotal) {
+ this.slotsTotal = slotsTotal;
+ }
+
+ public int getSlotsUsed() {
+ return slotsUsed;
+ }
+
+ public void setSlotsUsed(int slotsUsed) {
+ this.slotsUsed = slotsUsed;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ SupervisorStatus that = (SupervisorStatus) o;
+
+ if (getSlotsTotal() != that.getSlotsTotal()) {
+ return false;
+ }
+ if (getSlotsUsed() != that.getSlotsUsed()) {
+ return false;
+ }
+ if (!getId().equals(that.getId())) {
+ return false;
+ }
+ if (!getHost().equals(that.getHost())) {
+ return false;
+ }
+ return getUpTime().equals(that.getUpTime());
+ }
+
+ @Override
+ public int hashCode() {
+ int result = id != null? id.hashCode() : 0;
+ result = 31 * result + host != null? host.hashCode() : 0;
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129692080
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -17,10 +17,7 @@
*/
package org.apache.metron.rest.service.impl;
-import org.apache.metron.rest.model.TopologyResponse;
-import org.apache.metron.rest.model.TopologyStatus;
-import org.apache.metron.rest.model.TopologyStatusCode;
-import org.apache.metron.rest.model.TopologySummary;
+import org.apache.metron.rest.model.*;
--- End diff --
Most coding standards (including ours) don't like wildcard imports.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r120106798
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
What if the storm UI is https?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r120367143
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -49,6 +44,11 @@ public StormStatusServiceImpl(Environment environment, RestTemplate restTemplate
}
@Override
+ public SupervisorSummary getSupervisorSummary(){
+ return restTemplate.getForObject("http://" + environment.getProperty(STORM_UI_SPRING_PROPERTY) + SUPERVISOR_SUMMARY_URL, SupervisorSummary.class);
--- End diff --
https://github.com/apache/metron/pull/602 changes how that works slightly. Whichever one goes in second will need to adjust for the other one (which shouldn't be much work either way). I don't have a preference on ordering, personally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #607: METRON-982 add new rest api for storm supervisor s...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/607#discussion_r129908796
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/StormStatusServiceImpl.java ---
@@ -17,10 +17,7 @@
*/
package org.apache.metron.rest.service.impl;
-import org.apache.metron.rest.model.TopologyResponse;
-import org.apache.metron.rest.model.TopologyStatus;
-import org.apache.metron.rest.model.TopologyStatusCode;
-import org.apache.metron.rest.model.TopologySummary;
+import org.apache.metron.rest.model.*;
--- End diff --
Not to interrupt you guys....
I'm OK with changing this.... I have tried to setup intellij to not do this ( and I am already checking style ), but I think this PR came before I set things up.
--- OK, GO!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---