bonampak commented on code in PR #1395:
URL: https://github.com/apache/knox/pull/1395#discussion_r4327981269
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java:
##########
@@ -340,25 +344,31 @@ private boolean hasConfigChanged(String address, String
clusterName, List<Releva
// 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);
- }
+ // Get the current (model-derived) config for the started service.
This is null when the service produces no
+ // model (e.g. invalid configuration), just as such a service is
absent from the recorded baseline.
+ ServiceConfigurationModel currentConfig =
+ getCurrentServiceConfiguration(address, clusterName,
re.getService(), re.getServiceType());
+
+ if (serviceConfig == null && currentConfig == null) {
+ // Was and remains in an invalid configuration state (no model
either time): nothing to proxy, no change.
+ log.skippingConfigChangeForInvalidService(re.getService(),
re.getServiceType());
+ } else if (serviceConfig != null && currentConfig != null) {
+ // Valid before and now: compare the recorded and current configs to
detect a change.
+ 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.
+ } else if (currentConfig != null) {
+ // No prior config, but the service now produces a model: new /
became valid -> re-discover.
log.serviceEnabled(re.getService());
configHasChanged = true;
+ } else {
+ // Had a prior config but produces no model now: became invalid /
was removed -> re-discover so the
+ // service is dropped from the affected topologies (and the
scoped-replace merge clears its baseline).
+ log.serviceDisabled(re.getService());
+ configHasChanged = true;
Review Comment:
Fixed by
- 4b0c34209 returns a three-state result so an unreachable CM no longer
forces a rediscovery
- 37f71bfdb adds the tests.
One limitation, which also existed on master: after an error the event is
still marked processed,
so a real change that coincides with an outage isn't retried until another
event arrives. It is not a regression.
I'd create a follow-up ticket for that.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
