You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/04/07 23:44:48 UTC
[GitHub] [knox] lmccay opened a new pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
lmccay opened a new pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307
…all relevant CM event types
Change-Id: Ic26ad36fcb110e01d30d636f14d5b383de01ff17
(It is very **important** that you created an Apache Knox JIRA for this change and that the PR title/commit message includes the Apache Knox JIRA ID!)
This patch adds support for CM Event commands of type Start in addition to the existing Restart.
This is to account for services that are added and started but not restarted and discovery needing to be aware of these in case the service in question has been enabled in a descriptor.
Updated existing unit test to include Starts and ran all existing unit tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405598727
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
+ String command = null;
+ String status = null;
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
+ restartEvents.add(new RestartEvent(event));
+ }
+ }
+
+ private Map<String, Object> getAttributeMap(List<ApiEventAttribute> attributes) {
+ Map<String,Object> map = new HashMap<>();
+ attributes.forEach(attr -> { map.put(attr.getName(), attr.getValues());});
Review comment:
Yes, the map is redone for each event purposely. It is only a map of the attributes for the given event. The order of the events hasn't changed as a result of this patch and is driven by the REST API response and has nothing to do with the creation of this map. It works as intended.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405723167
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -360,19 +370,43 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
// Record the new event query timestamp for this address/cluster
setEventQueryTimestamp(address, clusterName, Instant.now());
- // Query the event log from CM for service/cluster restart events
- List<ApiEvent> events = queryRestartEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
+ // Query the event log from CM for service/cluster start events
+ List<ApiEvent> events = queryEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ if(isRelevantEvent(event)) {
+ relevantEvents.add(new StartEvent(event));
+ }
+ }
+
+ return relevantEvents;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isRelevantEvent(ApiEvent event) {
+ boolean rc = false;
+ String command = null;
+ String status = null;
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
Review comment:
IIRC, && does have a higher precedence than ||, which could mean the default grouping is `START_COMMAND.equals(command) || (RESTART_COMMAND.equals(command) &&
SUCCEEDED_STATUS.equals(status)) || STARTED_STATUS.equals(status)`, which probably still works in this case
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405643342
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
+ String command = null;
+ String status = null;
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
+ restartEvents.add(new RestartEvent(event));
+ }
+ }
+
+ private Map<String, Object> getAttributeMap(List<ApiEventAttribute> attributes) {
+ Map<String,Object> map = new HashMap<>();
+ attributes.forEach(attr -> { map.put(attr.getName(), attr.getValues());});
Review comment:
I think the latest revision will make the above more clear, Kevin. Thanks for the review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405732710
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -360,19 +370,43 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
// Record the new event query timestamp for this address/cluster
setEventQueryTimestamp(address, clusterName, Instant.now());
- // Query the event log from CM for service/cluster restart events
- List<ApiEvent> events = queryRestartEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
+ // Query the event log from CM for service/cluster start events
+ List<ApiEvent> events = queryEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ if(isRelevantEvent(event)) {
+ relevantEvents.add(new StartEvent(event));
+ }
+ }
+
+ return relevantEvents;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isRelevantEvent(ApiEvent event) {
+ boolean rc = false;
+ String command = null;
+ String status = null;
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
Review comment:
Not at this point. They won't be mixed up between the two and even if they were they both mean success.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405524259
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
Review comment:
Why not loop through the ApiEventAttribute list directly? There doesn't seem to be any value in converting to a Map. You break the map apart in the same way you would have to interrogate the ApiEventAttribute anyway.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405598727
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
+ String command = null;
+ String status = null;
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
+ restartEvents.add(new RestartEvent(event));
+ }
+ }
+
+ private Map<String, Object> getAttributeMap(List<ApiEventAttribute> attributes) {
+ Map<String,Object> map = new HashMap<>();
+ attributes.forEach(attr -> { map.put(attr.getName(), attr.getValues());});
Review comment:
Yes, the map is redone for each event purposely. It is only a map of the attributes for the given event. The order of the events hasn't changed as a result of this patch and is driven by the REST API response and has nothing to do with the creation of this map. It works as intended.
Regarding your question SERVICEA and SERVICEB - both events are preserved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405577731
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
Review comment:
Hmm I guess I am now more concerned with the logic here potentially then. I'll comment on the specific section below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405642911
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
Review comment:
Latest revision addresses the "restart" terminology and cleans up the method as you suggested offline. Thanks for the review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405720842
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -360,19 +370,43 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
// Record the new event query timestamp for this address/cluster
setEventQueryTimestamp(address, clusterName, Instant.now());
- // Query the event log from CM for service/cluster restart events
- List<ApiEvent> events = queryRestartEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
+ // Query the event log from CM for service/cluster start events
+ List<ApiEvent> events = queryEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ if(isRelevantEvent(event)) {
+ relevantEvents.add(new StartEvent(event));
+ }
+ }
+
+ return relevantEvents;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isRelevantEvent(ApiEvent event) {
+ boolean rc = false;
+ String command = null;
+ String status = null;
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
Review comment:
I could see this being grouped as
`(START_COMMAND.equals(command) && STARTED_STATUS.equals(status)) || (RESTART_COMMAND.equals(command) && SUCCEEDED_STATUS.equals(status))`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405724123
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -360,19 +370,43 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
// Record the new event query timestamp for this address/cluster
setEventQueryTimestamp(address, clusterName, Instant.now());
- // Query the event log from CM for service/cluster restart events
- List<ApiEvent> events = queryRestartEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
+ // Query the event log from CM for service/cluster start events
+ List<ApiEvent> events = queryEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ if(isRelevantEvent(event)) {
+ relevantEvents.add(new StartEvent(event));
+ }
+ }
+
+ return relevantEvents;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isRelevantEvent(ApiEvent event) {
+ boolean rc = false;
+ String command = null;
+ String status = null;
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
Review comment:
It also may pick up _any_ event type that has a status value of `STARTED_STATUS`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405701987
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -360,19 +370,43 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
// Record the new event query timestamp for this address/cluster
setEventQueryTimestamp(address, clusterName, Instant.now());
- // Query the event log from CM for service/cluster restart events
- List<ApiEvent> events = queryRestartEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
+ // Query the event log from CM for service/cluster start events
+ List<ApiEvent> events = queryEvents(getApiClient(configCache.getDiscoveryConfig(address, clusterName)),
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ if(isRelevantEvent(event)) {
+ relevantEvents.add(new StartEvent(event));
+ }
+ }
+
+ return relevantEvents;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean isRelevantEvent(ApiEvent event) {
+ boolean rc = false;
+ String command = null;
+ String status = null;
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
Review comment:
Does this logic need to be grouped somehow? Right now this is mixing && and ||?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405592686
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
+ String command = null;
+ String status = null;
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
+ restartEvents.add(new RestartEvent(event));
+ }
+ }
+
+ private Map<String, Object> getAttributeMap(List<ApiEventAttribute> attributes) {
+ Map<String,Object> map = new HashMap<>();
+ attributes.forEach(attr -> { map.put(attr.getName(), attr.getValues());});
Review comment:
The map is associated with a specific ApiEvent, so I'm not sure that the map is the concern. We may want to sort the ApiEvent collection by _timeOccurred_ to avoid issues associated with what you're describing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405595229
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
Review comment:
Since the map is derived from the ApiEvent, addIfRelevantEvent should not need the attribute Map passed in as a param.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405529175
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
Review comment:
These aren't only restart events at this point, correct? Perhaps, RestartEvent should be renamed to ConfigChangeEvent or something more appropriate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405601417
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
Review comment:
I didn't see any point in changing it to reflect Start and Restart the are both "start" events. We can change to Start which is even a semantically meaningless change. I wanted to keep the map external rather than create it inside that method in case we need to add an additional use of the map later. We can move it in and refactor later if needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405572741
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
Review comment:
That is what I had before and it is much more complicated as I wanted to avoid creating a map. Note that we are checking two correlated attributes. The command issued and its status. Since you don't know what order they are in you have to squirrel them away then check whether you have both then add. If we end up having to add more events that would be even messier to do. Now, we can get the map upfront and do whatever processing of it afterward much more cleanly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307#discussion_r405579448
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -365,12 +375,32 @@ private DiscoveryApiClient getApiClient(final ServiceDiscoveryConfig discoveryCo
clusterName,
lastTimestamp);
for (ApiEvent event : events) {
- restartEvents.add(new RestartEvent(event));
+ List<ApiEventAttribute> attributes = event.getAttributes();
+ Map<String,Object> map = getAttributeMap(attributes);
+ addIfRelevantEvent(restartEvents, event, map);
}
return restartEvents;
}
+ @SuppressWarnings("unchecked")
+ private void addIfRelevantEvent(List<RestartEvent> restartEvents, ApiEvent event, Map<String, Object> map) {
+ String command = null;
+ String status = null;
+ command = (String) ((List<String>) map.get(COMMAND)).get(0);
+ status = (String) ((List<String>) map.get(COMMAND_STATUS)).get(0);
+ if (START_COMMAND.equals(command) || RESTART_COMMAND.equals(command) &&
+ SUCCEEDED_STATUS.equals(status) || STARTED_STATUS.equals(status)) {
+ restartEvents.add(new RestartEvent(event));
+ }
+ }
+
+ private Map<String, Object> getAttributeMap(List<ApiEventAttribute> attributes) {
+ Map<String,Object> map = new HashMap<>();
+ attributes.forEach(attr -> { map.put(attr.getName(), attr.getValues());});
Review comment:
This will overwrite in the map correct? The attribute get name - is that unique? I think this is by COMMAND or COMMAND_STATUS based on the logic for eventually checking the map. If there are duplicate commands won't they get overwritten in the map? So its last write wins?
If you do a Start of SERVICEA and then Start of SERVICEB - will you see both events or just collapse it down to one?
Another example is doing start of same service twice with different results. start success then (implicit stop) then start failure. Will that get picked up?
I think the map is totally breaking the uniqueness and ordering of the events.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] lmccay merged pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
Posted by GitBox <gi...@apache.org>.
lmccay merged pull request #307: KNOX-2304 - CM discovery cluster config monitor needs to be aware of …
URL: https://github.com/apache/knox/pull/307
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services