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/22 15:16:01 UTC
[GitHub] [knox] risdenk commented on a change in pull request #324: KNOX-2351 - Catching any errors while monitoring CM configuration changes
risdenk commented on a change in pull request #324:
URL: https://github.com/apache/knox/pull/324#discussion_r413073076
##########
File path: gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java
##########
@@ -162,93 +162,98 @@ public void run() {
isActive = true;
while (isActive) {
- List<String> clustersToStopMonitoring = new ArrayList<>();
+ try {
+ final List<String> clustersToStopMonitoring = new ArrayList<>();
+
+ for (Map.Entry<String, List<String>> entry : configCache.getClusterNames().entrySet()) {
+ String address = entry.getKey();
+ for (String clusterName : entry.getValue()) {
+ log.checkingClusterConfiguration(clusterName, address);
+
+ // Check here for existing descriptor references, and add to the removal list if there are not any
+ if (!clusterReferencesExist(address, clusterName)) {
+ clustersToStopMonitoring.add(address + FQCN_DELIM + clusterName);
+ continue;
+ }
- for (Map.Entry<String, List<String>> entry : configCache.getClusterNames().entrySet()) {
- String address = entry.getKey();
- for (String clusterName : entry.getValue()) {
- log.checkingClusterConfiguration(clusterName, address);
+ // Configuration changes don't mean anything without corresponding service start/restarts. Therefore, monitor
+ // start events, and check the configuration only of the restarted service(s) to identify changes
+ // that should trigger re-discovery.
+ final List<StartEvent> relevantEvents = getRelevantEvents(address, clusterName);
- // Check here for existing descriptor references, and add to the removal list if there are not any
- if (!clusterReferencesExist(address, clusterName)) {
- clustersToStopMonitoring.add(address + FQCN_DELIM + clusterName);
- continue;
+ // If there are no recent start events, then nothing to do now
+ if (!relevantEvents.isEmpty()) {
+ // If a change has occurred, notify the listeners
+ if (hasConfigChanged(address, clusterName, relevantEvents)) {
+ notifyChangeListener(address, clusterName);
+ }
+ }
}
+ }
- // Configuration changes don't mean anything without corresponding service start/restarts. Therefore, monitor
- // start events, and check the configuration only of the restarted service(s) to identify changes
- // that should trigger re-discovery.
- List<StartEvent> relevantEvents = getRelevantEvents(address, clusterName);
-
- // If there are no recent start events, then nothing to do now
- if (!relevantEvents.isEmpty()) {
- boolean configHasChanged = false;
-
- // If there are start events, then check the previously-recorded properties for the same service to
- // identify if the configuration has changed
- Map<String, ServiceConfigurationModel> serviceConfigurations =
- configCache.getClusterServiceConfigurations(address, clusterName);
-
- // Those services for which a start even has been handled
- List<String> handledServiceTypes = new ArrayList<>();
-
- for (StartEvent re : relevantEvents) {
- String serviceType = re.getServiceType();
-
- // Determine if we've already handled a start event for this service type
- if (!handledServiceTypes.contains(serviceType)) {
-
- // Get the previously-recorded configuration
- ServiceConfigurationModel serviceConfig = serviceConfigurations.get(re.getServiceType());
-
- if (serviceConfig != null) {
- // Get the current config for the started service, and compare with the previously-recorded config
- ServiceConfigurationModel currentConfig =
- getCurrentServiceConfiguration(address, clusterName, re.getService());
-
- if (currentConfig != null) {
- log.analyzingCurrentServiceConfiguration(re.getService());
- try {
- configHasChanged = hasConfigurationChanged(serviceConfig, currentConfig);
- } catch (Exception e) {
- log.errorAnalyzingCurrentServiceConfiguration(re.getService(), e);
- }
- }
- } else {
- // A new service (no prior config) represent a config change, since a descriptor may have referenced
- // the "new" service, but discovery had previously not succeeded because the service had not been
- // configured (appropriately) at that time.
- log.serviceEnabled(re.getService());
- configHasChanged = true;
- }
-
- handledServiceTypes.add(serviceType);
- }
+ // Remove outdated entries from the cache
+ for (String fqcn : clustersToStopMonitoring) {
+ String[] parts = fqcn.split(FQCN_DELIM);
+ stopMonitoring(parts[0], parts[1]);
+ }
+ clustersToStopMonitoring.clear(); // reset the removal list
- if (configHasChanged) {
- break; // No need to continue checking once we've identified one reason to perform discovery again
- }
- }
+ waitFor(interval);
+ } catch (Throwable t) {
Review comment:
Why not just Exception? This would also catch runtime exceptions? Seems overly broad?
----------------------------------------------------------------
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