Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor reload (2) #1790

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Refactor reload (2) #1790

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f56026f
dirty
wind57 Nov 8, 2024
36aeb02
return early
wind57 Nov 8, 2024
8a48b7e
add return
wind57 Nov 8, 2024
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
25 changes: 22 additions & 3 deletions ...ain/java/org/springframework/cloud/kubernetes/commons/config/reload/ConfigReloadUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,30 @@ private ConfigReloadUtil() {

private static final LogAccessor LOG = new LogAccessor(LogFactory.getLog(ConfigReloadUtil.class));

public static boolean reload(String target, String eventSourceType, PropertySourceLocator locator,
/**
* used for the event based reloading.
*/
public static boolean reload(String target, String sourceAsString, PropertySourceLocator locator,
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 want to achieve a very simple thing here : only two single "entry points" into this class. Only two public methods that both polling and event based reload should call, nothing else should be public. And they do exactly the same thing

ConfigurableEnvironment environment, Class<? extends MapPropertySource> existingSourcesType) {
LOG.debug(() -> "onEvent " + target + ": " + eventSourceType);
LOG.debug(() -> "onEvent " + target + ": " + sourceAsString);

return reload(locator, environment, existingSourcesType);
}

/**
* used for the poll based reloading.
*/
public static boolean reload(PropertySourceLocator locator, ConfigurableEnvironment environment,
Class<? extends MapPropertySource> existingSourcesType) {

List<? extends MapPropertySource> sourceFromK8s = locateMapPropertySources(locator, environment);
List<? extends MapPropertySource> existingSources = findPropertySources(existingSourcesType, environment);

if (existingSources.isEmpty()) {
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 am adding this code:

if (existingSources.isEmpty()) {
			LOG.debug(() -> "no existingSources found, reload will not happen");
            return false;
}

This was actually already part of the polling code, but not from the event based one. So this aligns them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR can only be created once you are OK with this one and it ends up in main...

LOG.debug(() -> "no existingSources found, reload will not happen");
}

List<? extends MapPropertySource> sourceFromK8s = locateMapPropertySources(locator, environment);

boolean changed = changed(sourceFromK8s, existingSources);
if (changed) {
LOG.info("Detected change in config maps/secrets");
Expand All @@ -67,7 +84,9 @@ public static boolean reload(String target, String eventSourceType, PropertySour
* @param <S> property source type
* @param sourceClass class for which property sources will be found
* @return finds all registered property sources of the given type
* @deprecated this method will not be public in the next major release.
*/
@Deprecated(forRemoval = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this is public already, I can't make it package private, but I will create a PR that will make it such for the next major release

public static <S extends PropertySource<?>> List<S> findPropertySources(Class<S> sourceClass,
ConfigurableEnvironment environment) {
List<S> managedSources = new ArrayList<>();
Expand Down
23 changes: 4 additions & 19 deletions ...pringframework/cloud/kubernetes/commons/config/reload/PollingConfigMapChangeDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.springframework.cloud.kubernetes.commons.config.reload;

import java.time.Duration;
import java.util.List;

import jakarta.annotation.PostConstruct;
import org.apache.commons.logging.Log;
Expand All @@ -29,10 +28,6 @@
import org.springframework.scheduling.TaskScheduler;
import org.springframework.scheduling.support.PeriodicTrigger;

import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.changed;
import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.findPropertySources;
import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.locateMapPropertySources;

/**
* A change detector that periodically retrieves configmaps and fire a reload when
* something changes.
Expand Down Expand Up @@ -75,23 +70,13 @@ private void init() {
}

private void executeCycle() {

boolean changedConfigMap = false;
if (monitorConfigMaps) {
log.debug("Polling for changes in config maps");
List<? extends MapPropertySource> currentConfigMapSources = findPropertySources(propertySourceClass,
environment);

if (!currentConfigMapSources.isEmpty()) {
changedConfigMap = changed(locateMapPropertySources(this.propertySourceLocator, this.environment),
currentConfigMapSources);
boolean changedConfigMap = ConfigReloadUtil.reload(propertySourceLocator, environment, propertySourceClass);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of calling : findPropertySources, then locateMapPropertySources, we now call reload, that internally does the same thing

if (changedConfigMap) {
log.info("Detected change in config maps");
reloadProperties();
}
}

if (changedConfigMap) {
log.info("Detected change in config maps");
reloadProperties();
}
}

}
23 changes: 4 additions & 19 deletions .../springframework/cloud/kubernetes/commons/config/reload/PollingSecretsChangeDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.springframework.cloud.kubernetes.commons.config.reload;

import java.time.Duration;
import java.util.List;

import jakarta.annotation.PostConstruct;
import org.apache.commons.logging.Log;
Expand All @@ -29,10 +28,6 @@
import org.springframework.scheduling.TaskScheduler;
import org.springframework.scheduling.support.PeriodicTrigger;

import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.changed;
import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.findPropertySources;
import static org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadUtil.locateMapPropertySources;

/**
* A change detector that periodically retrieves secrets and fires a reload when something
* changes.
Expand Down Expand Up @@ -75,23 +70,13 @@ private void init() {
}

private void executeCycle() {

boolean changedSecrets = false;
if (monitorSecrets) {
log.debug("Polling for changes in secrets");
List<MapPropertySource> currentSecretSources = locateMapPropertySources(this.propertySourceLocator,
this.environment);
if (!currentSecretSources.isEmpty()) {
List<? extends MapPropertySource> propertySources = findPropertySources(propertySourceClass,
environment);
changedSecrets = changed(currentSecretSources, propertySources);
boolean changedSecrets = ConfigReloadUtil.reload(propertySourceLocator, environment, propertySourceClass);
if (changedSecrets) {
log.info("Detected change in secrets");
reloadProperties();
}
}

if (changedSecrets) {
log.info("Detected change in secrets");
reloadProperties();
}
}

}
Loading