Skip to content

fix: naming and minor structural improvements #1130

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.

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

Merged
merged 6 commits into from
Apr 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,36 +20,36 @@ public interface InformerConfiguration<R extends HasMetadata, P extends HasMetad
class DefaultInformerConfiguration<R extends HasMetadata, P extends HasMetadata> extends
DefaultResourceConfiguration<R> implements InformerConfiguration<R, P> {

private final SecondaryToPrimaryMapper<R> secondaryToPrimaryResourcesIdSet;
private final PrimaryToSecondaryMapper<P> associatedWith;
private final SecondaryToPrimaryMapper<R> secondaryToPrimaryMapper;
private final PrimaryToSecondaryMapper<P> primaryToSecondaryMapper;

protected DefaultInformerConfiguration(String labelSelector,
Class<R> resourceClass,
SecondaryToPrimaryMapper<R> secondaryToPrimaryResourcesIdSet,
PrimaryToSecondaryMapper<P> associatedWith,
SecondaryToPrimaryMapper<R> secondaryToPrimaryMapper,
PrimaryToSecondaryMapper<P> primaryToSecondaryMapper,
Set<String> namespaces) {
super(labelSelector, resourceClass, namespaces);
this.secondaryToPrimaryResourcesIdSet =
Objects.requireNonNullElse(secondaryToPrimaryResourcesIdSet,
this.secondaryToPrimaryMapper =
Objects.requireNonNullElse(secondaryToPrimaryMapper,
Mappers.fromOwnerReference());
this.associatedWith =
Objects.requireNonNullElseGet(associatedWith, () -> ResourceID::fromResource);
this.primaryToSecondaryMapper =
Objects.requireNonNullElseGet(primaryToSecondaryMapper, () -> ResourceID::fromResource);
}


public SecondaryToPrimaryMapper<R> getPrimaryResourcesRetriever() {
return secondaryToPrimaryResourcesIdSet;
public SecondaryToPrimaryMapper<R> getSecondaryToPrimaryMapper() {
return secondaryToPrimaryMapper;
}

public PrimaryToSecondaryMapper<P> getAssociatedResourceIdentifier() {
return associatedWith;
public PrimaryToSecondaryMapper<P> getPrimaryToSecondaryMapper() {
return primaryToSecondaryMapper;
}

}

SecondaryToPrimaryMapper<R> getPrimaryResourcesRetriever();
SecondaryToPrimaryMapper<R> getSecondaryToPrimaryMapper();

PrimaryToSecondaryMapper<P> getAssociatedResourceIdentifier();
PrimaryToSecondaryMapper<P> getPrimaryToSecondaryMapper();

@SuppressWarnings("unused")
class InformerConfigurationBuilder<R extends HasMetadata, P extends HasMetadata> {
Expand Down Expand Up @@ -116,7 +116,7 @@ static <R extends HasMetadata, P extends HasMetadata> InformerConfigurationBuild
.withNamespaces(configuration.getNamespaces())
.withLabelSelector(configuration.getLabelSelector())
.withAssociatedSecondaryResourceIdentifier(
configuration.getAssociatedResourceIdentifier())
.withPrimaryResourcesRetriever(configuration.getPrimaryResourcesRetriever());
configuration.getPrimaryToSecondaryMapper())
.withPrimaryResourcesRetriever(configuration.getSecondaryToPrimaryMapper());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ public Stream<R> list(Predicate<R> predicate) {
return cache.list(predicate);
}



protected UpdatableCache<R> initCache() {
return new ConcurrentHashMapCache<>();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@

@FunctionalInterface
public interface PrimaryToSecondaryMapper<P extends HasMetadata> {
ResourceID associatedSecondaryID(P primary);
ResourceID toSecondaryResourceID(P primary);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@

@FunctionalInterface
public interface SecondaryToPrimaryMapper<T> {
Set<ResourceID> associatedPrimaryResources(T dependentResource);
Set<ResourceID> toPrimaryResourceIDs(T dependentResource);
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,6 @@ private void handleKubernetesClientException(Exception e) {

@Override
public Optional<T> getSecondaryResource(T primary) {
return get(ResourceID.fromResource(primary));
throw new IllegalStateException("This method should not be called here. Primary: " + primary);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not makes sense imo to ask for the secondary resource from the ControllerEventSource. It basically does not have secondary, so this operation should fail.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,17 @@ public InformerEventSource(InformerConfiguration<R, P> configuration, Kubernetes

@Override
public void onAdd(R resource) {
if (log.isDebugEnabled()) {
log.debug("On add event received for resource id: {}", ResourceID.fromResource(resource));
}
onAddOrUpdate("add", resource, () -> InformerEventSource.super.onAdd(resource));
}

@Override
public void onUpdate(R oldObject, R newObject) {
if (log.isDebugEnabled()) {
log.debug("On update event received for resource id: {}", ResourceID.fromResource(newObject));
}
onAddOrUpdate("update", newObject,
() -> InformerEventSource.super.onUpdate(oldObject, newObject));
}
Expand Down Expand Up @@ -118,14 +124,17 @@ private synchronized void onAddOrUpdate(String operation, R newObject, Runnable
}

@Override
public void onDelete(R r, boolean b) {
super.onDelete(r, b);
propagateEvent(r);
public void onDelete(R resource, boolean b) {
if (log.isDebugEnabled()) {
log.debug("On delete event received for resource id: {}", ResourceID.fromResource(resource));
}
super.onDelete(resource, b);
propagateEvent(resource);
}

private void propagateEvent(R object) {
var primaryResourceIdSet =
configuration.getPrimaryResourcesRetriever().associatedPrimaryResources(object);
configuration.getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object);
if (primaryResourceIdSet.isEmpty()) {
return;
}
Expand Down Expand Up @@ -153,7 +162,7 @@ private void propagateEvent(R object) {
*/
@Override
public Optional<R> getSecondaryResource(P resource) {
final var id = configuration.getAssociatedResourceIdentifier().associatedSecondaryID(resource);
final var id = configuration.getPrimaryToSecondaryMapper().toSecondaryResourceID(resource);
return get(id);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,7 @@ public Optional<R> get(ResourceID resourceID) {
}

@Override
public Optional<R> getSecondaryResource(P primary) {
return get(ResourceID.fromResource(primary));
}
public abstract Optional<R> getSecondaryResource(P primary);

@Override
public Optional<R> getCachedValue(ResourceID resourceID) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public synchronized void putAddedResource(T newResource) {
ResourceID resourceID = ResourceID.fromResource(newResource);
if (managedInformerEventSource.get(resourceID).isEmpty()) {
log.debug("Putting resource to cache with ID: {}", resourceID);
cache.put(ResourceID.fromResource(newResource), newResource);
cache.put(resourceID, newResource);
} else {
log.debug("Won't put resource into cache found already informer cache: {}", resourceID);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,17 @@ void setup() {
.thenReturn(labeledResourceClientMock);
when(labeledResourceClientMock.runnableInformer(0)).thenReturn(informer);

when(informerConfiguration.getPrimaryResourcesRetriever())
when(informerConfiguration.getSecondaryToPrimaryMapper())
.thenReturn(mock(SecondaryToPrimaryMapper.class));

informerEventSource = new InformerEventSource<>(informerConfiguration, clientMock);
informerEventSource.setTemporalResourceCache(temporaryResourceCacheMock);
informerEventSource.setEventHandler(eventHandlerMock);

SecondaryToPrimaryMapper secondaryToPrimaryMapper = mock(SecondaryToPrimaryMapper.class);
when(informerConfiguration.getPrimaryResourcesRetriever())
when(informerConfiguration.getSecondaryToPrimaryMapper())
.thenReturn(secondaryToPrimaryMapper);
when(secondaryToPrimaryMapper.associatedPrimaryResources(any()))
when(secondaryToPrimaryMapper.toPrimaryResourceIDs(any()))
.thenReturn(Set.of(ResourceID.fromResource(testDeployment())));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected ConfigMap desired(OrderedManagedDependentCustomResource primary,
}

@Override
public ResourceID associatedSecondaryID(OrderedManagedDependentCustomResource primary) {
public ResourceID toSecondaryResourceID(OrderedManagedDependentCustomResource primary) {
return new ResourceID(primary.getMetadata().getName() + "1",
primary.getMetadata().getNamespace());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected ConfigMap desired(OrderedManagedDependentCustomResource primary,
}

@Override
public ResourceID associatedSecondaryID(OrderedManagedDependentCustomResource primary) {
public ResourceID toSecondaryResourceID(OrderedManagedDependentCustomResource primary) {
return new ResourceID(primary.getMetadata().getName() + "2",
primary.getMetadata().getNamespace());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public Result<Secret> match(Secret actual, MySQLSchema primary, Context<MySQLSch
}

@Override
public ResourceID associatedSecondaryID(MySQLSchema primary) {
public ResourceID toSecondaryResourceID(MySQLSchema primary) {
return new ResourceID(
getSecretName(primary.getMetadata().getName()), primary.getMetadata().getNamespace());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public ConfigMap update(ConfigMap actual, ConfigMap target, WebPage primary,
}

@Override
public ResourceID associatedSecondaryID(WebPage primary) {
public ResourceID toSecondaryResourceID(WebPage primary) {
return new ResourceID(configMapName(primary), primary.getMetadata().getNamespace());
}
}