Skip to content

refactor: clean some reported code smells #1660

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 2 commits into from
Dec 14, 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 @@ -55,6 +55,7 @@ public void controllerRegistered(Controller<?> controller) {
gauges.put(controllerQueueName, controllerQueueSize);
}

@Override
public <T> T timeControllerExecution(ControllerExecution<T> execution) {
final var name = execution.controllerName();
final var execName = PREFIX + "controllers.execution." + execution.name();
Expand All @@ -65,7 +66,7 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
"controller", name,
"resource.name", resourceID.getName(),
"resource.namespace", resourceID.getNamespace().orElse(""),
"resource.scope", resourceID.getNamespace().isPresent() ? "namespace" : "cluster"));
"resource.scope", getScope(resourceID)));
final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY);
if (gvk != null) {
tags.addAll(List.of(
Expand Down Expand Up @@ -101,6 +102,11 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
}
}

private static String getScope(ResourceID resourceID) {
return resourceID.getNamespace().isPresent() ? "namespace" : "cluster";
}

@Override
public void receivedEvent(Event event, Map<String, Object> metadata) {
incrementCounter(event.getRelatedCustomResourceID(), "events.received",
metadata,
Expand Down Expand Up @@ -151,6 +157,7 @@ public void reconciliationExecutionFinished(HasMetadata resource, Map<String, Ob
controllerQueueSize.decrementAndGet();
}

@Override
public void failedReconciliation(HasMetadata resource, Exception exception,
Map<String, Object> metadata) {
var cause = exception.getCause();
Expand All @@ -164,6 +171,7 @@ public void failedReconciliation(HasMetadata resource, Exception exception,
cause.getClass().getSimpleName());
}

@Override
public <T extends Map<?, ?>> T monitorSizeOf(T map, String name) {
return registry.gaugeMapSize(PREFIX + name + ".size", Collections.emptyList(), map);
}
Expand All @@ -183,7 +191,7 @@ private void incrementCounter(ResourceID id, String counterName, Map<String, Obj
tags.addAll(List.of(
"name", id.getName(),
"namespace", id.getNamespace().orElse(""),
"scope", id.getNamespace().isPresent() ? "namespace" : "cluster"));
"scope", getScope(id)));
if (additionalTagsNb > 0) {
tags.addAll(List.of(additionalTags));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition;

public abstract class CustomResourceUtils {
public class CustomResourceUtils {

private CustomResourceUtils() {}

/**
* Applies internal validations that may not be handled by the fabric8 client.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import io.fabric8.kubernetes.client.Version;
import io.javaoperatorsdk.operator.api.config.*;
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceOverrider;
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.ControllerConfigurationOverrider;
import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.processing.LifecycleAware;
Expand Down Expand Up @@ -112,9 +117,8 @@ public synchronized void start() {
leaderElectionManager.start();
started = true;
} catch (Exception e) {
log.error("Error starting operator", e);
stop();
throw e;
throw new OperatorException("Error starting operator", e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,11 @@ public static void handleKubernetesClientException(Exception e, String resourceT

if (e instanceof KubernetesClientException) {
KubernetesClientException ke = (KubernetesClientException) e;
if (404 == ke.getCode()) {
// only throw MissingCRDException if the 404 error occurs on the target CRD
if (resourceTypeName.equals(ke.getFullResourceName())
|| matchesResourceType(resourceTypeName, ke)) {
throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
}
// only throw MissingCRDException if the 404 error occurs on the target CRD
if (404 == ke.getCode() &&
(resourceTypeName.equals(ke.getFullResourceName())
|| matchesResourceType(resourceTypeName, ke))) {
throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ public ResourceEventFilter<P> getEventFilter() {
return eventFilter;
}

/**
* @deprecated Use {@link OnAddFilter}, {@link OnUpdateFilter} and {@link GenericFilter} instead
*/
@Deprecated(forRemoval = true)
protected void setEventFilter(ResourceEventFilter<P> eventFilter) {
this.eventFilter = eventFilter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ public class Utils {
private static final Logger log = LoggerFactory.getLogger(Utils.class);
public static final String CHECK_CRD_ENV_KEY = "JAVA_OPERATOR_SDK_CHECK_CRD";
public static final String DEBUG_THREAD_POOL_ENV_KEY = "JAVA_OPERATOR_SDK_DEBUG_THREAD_POOL";
public static final String GENERIC_PARAMETER_TYPE_ERROR_PREFIX =
"Couldn't retrieve generic parameter type from ";

/**
* Attempts to load version information from a properties file produced at build time, currently
Expand Down Expand Up @@ -102,7 +104,7 @@ public static Class<?> getFirstTypeArgumentFromExtendedClass(Class<?> clazz) {
Type type = clazz.getGenericSuperclass();
return (Class<?>) ((ParameterizedType) type).getActualTypeArguments()[0];
} catch (Exception e) {
throw new RuntimeException("Couldn't retrieve generic parameter type from "
throw new RuntimeException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
+ clazz.getSimpleName()
+ " because it doesn't extend a class that is parameterized with the type we want to retrieve",
e);
Expand All @@ -118,49 +120,55 @@ public static Class<?> getTypeArgumentFromInterfaceByIndex(Class<?> clazz,
Class<?> expectedImplementedInterface, int index) {
if (expectedImplementedInterface.isAssignableFrom(clazz)) {
final var genericInterfaces = clazz.getGenericInterfaces();
Optional<? extends Class<?>> target = Optional.empty();
if (genericInterfaces.length > 0) {
// try to find the target interface among them
target = Arrays.stream(genericInterfaces)
.filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
&& type instanceof ParameterizedType)
.map(ParameterizedType.class::cast)
.findFirst()
.map(t -> {
final Type argument = t.getActualTypeArguments()[index];
if (argument instanceof Class) {
return (Class<?>) argument;
}
// account for the case where the argument itself has parameters, which we will ignore
// and just return the raw type
if (argument instanceof ParameterizedType) {
final var rawType = ((ParameterizedType) argument).getRawType();
if (rawType instanceof Class) {
return (Class<?>) rawType;
}
}
throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
+ expectedImplementedInterface.getSimpleName()
+ " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
+ argument);
});
}

var target = extractType(clazz, expectedImplementedInterface, index, genericInterfaces);
if (target.isPresent()) {
return target.get();
}

// try the parent
// try the parent if we didn't find a parameter type on the current class
var parent = clazz.getSuperclass();
if (!Object.class.equals(parent)) {
return getTypeArgumentFromInterfaceByIndex(parent, expectedImplementedInterface, index);
}
}
throw new IllegalArgumentException("Couldn't retrieve generic parameter type from "
throw new IllegalArgumentException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
+ clazz.getSimpleName() + " because it or its superclasses don't implement "
+ expectedImplementedInterface.getSimpleName());
}

private static Optional<? extends Class<?>> extractType(Class<?> clazz,
Class<?> expectedImplementedInterface, int index, Type[] genericInterfaces) {
Optional<? extends Class<?>> target = Optional.empty();
if (genericInterfaces.length > 0) {
// try to find the target interface among them
target = Arrays.stream(genericInterfaces)
.filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
&& type instanceof ParameterizedType)
.map(ParameterizedType.class::cast)
.findFirst()
.map(t -> {
final Type argument = t.getActualTypeArguments()[index];
if (argument instanceof Class) {
return (Class<?>) argument;
}
// account for the case where the argument itself has parameters, which we will ignore
// and just return the raw type
if (argument instanceof ParameterizedType) {
final var rawType = ((ParameterizedType) argument).getRawType();
if (rawType instanceof Class) {
return (Class<?>) rawType;
}
}
throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
+ expectedImplementedInterface.getSimpleName()
+ " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
+ argument);
});
}
return target;
}

public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> clazz,
Class<?> expectedImplementedInterface) {
// first check super class if it exists
Expand All @@ -183,7 +191,7 @@ public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> cl
return getFirstTypeArgumentFromInterface(clazz, expectedImplementedInterface);
} catch (Exception e) {
throw new OperatorException(
"Couldn't retrieve generic parameter type from " + clazz.getSimpleName(), e);
GENERIC_PARAMETER_TYPE_ERROR_PREFIX + clazz.getSimpleName(), e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ default void controllerRegistered(Controller<?> controller) {}
*/
default void receivedEvent(Event event, Map<String, Object> metadata) {}

/**
* @deprecated Use {@link #reconcileCustomResource(HasMetadata, RetryInfo, Map)} instead
*/
@Deprecated(forRemoval = true)
default void reconcileCustomResource(ResourceID resourceID, RetryInfo retryInfo,
Map<String, Object> metadata) {}
Expand All @@ -51,6 +54,9 @@ default void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfo,
reconcileCustomResource(ResourceID.fromResource(resource), retryInfo, metadata);
}

/**
* @deprecated Use {@link #failedReconciliation(HasMetadata, Exception, Map)} instead
*/
@Deprecated(forRemoval = true)
default void failedReconciliation(ResourceID resourceID, Exception exception,
Map<String, Object> metadata) {}
Expand Down Expand Up @@ -102,6 +108,9 @@ default void finishedReconciliation(ResourceID resourceID) {
finishedReconciliation(resourceID, Collections.emptyMap());
}

/**
* @deprecated Use {@link #finishedReconciliation(HasMetadata, Map)} instead
*/
@Deprecated(forRemoval = true)
default void finishedReconciliation(ResourceID resourceID, Map<String, Object> metadata) {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ public class DefaultManagedWorkflow<P extends HasMetadata> implements ManagedWor

private final Set<String> topLevelResources;
private final Set<String> bottomLevelResources;
private final List<DependentResourceSpec<?, ?>> orderedSpecs;
private final List<DependentResourceSpec> orderedSpecs;
private final boolean hasCleaner;

protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
protected DefaultManagedWorkflow(List<DependentResourceSpec> orderedSpecs,
boolean hasCleaner) {
this.hasCleaner = hasCleaner;
topLevelResources = new HashSet<>(orderedSpecs.size());
bottomLevelResources = orderedSpecs.stream()
.map(DependentResourceSpec::getName)
.collect(Collectors.toSet());
this.orderedSpecs = orderedSpecs;
orderedSpecs.forEach(spec -> {
for (DependentResourceSpec<?, ?> spec : orderedSpecs) {
// add cycle detection?
if (spec.getDependsOn().isEmpty()) {
topLevelResources.add(spec.getName());
Expand All @@ -42,12 +42,12 @@ protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
bottomLevelResources.remove(dependsOn);
}
}
});
}
}

@Override
@SuppressWarnings("unused")
public List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
public List<DependentResourceSpec> getOrderedSpecs() {
return orderedSpecs;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public DependentResourceNode(String name, Condition<R, P> reconcilePrecondition,
this.dependentResource = dependentResource;
}

public List<? extends DependentResourceNode> getDependsOn() {
public List<DependentResourceNode> getDependsOn() {
return dependsOn;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

public interface ManagedWorkflow<P extends HasMetadata> {

@SuppressWarnings("unused")
default List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
@SuppressWarnings({"unused", "rawtypes"})
default List<DependentResourceSpec> getOrderedSpecs() {
return Collections.emptyList();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,11 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
* @return top-bottom ordered resources that can be added safely to workflow
* @throws OperatorException if there is a cycle in the dependencies
*/
private List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
private List<DependentResourceSpec> orderAndDetectCycles(
List<DependentResourceSpec> dependentResourceSpecs, boolean[] cleanerHolder) {

final var drInfosByName = createDRInfos(dependentResourceSpecs);
final var orderedSpecs =
new ArrayList<DependentResourceSpec<?, ?>>(dependentResourceSpecs.size());
final var orderedSpecs = new ArrayList<DependentResourceSpec>(dependentResourceSpecs.size());
final var alreadyVisited = new HashSet<String>();
var toVisit = getTopDependentResources(dependentResourceSpecs);

Expand Down Expand Up @@ -108,7 +107,7 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
* @return top-bottom ordered resources that can be added safely to workflow
* @throws OperatorException if there is a cycle in the dependencies
*/
public List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
public List<DependentResourceSpec> orderAndDetectCycles(
List<DependentResourceSpec> dependentResourceSpecs) {
return orderAndDetectCycles(dependentResourceSpecs, null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ public void unMarkEventReceived() {
throw new IllegalStateException("Cannot unmark processed marked for deletion.");
case DELETE_EVENT_PRESENT:
throw new IllegalStateException("Cannot unmark delete event.");
case NO_EVENT_PRESENT:
// do nothing
break;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ private boolean isAcceptedByFilters(ResourceAction action, T resource, T oldReso
return onAddFilter == null || onAddFilter.accept(resource);
case UPDATED:
return onUpdateFilter.accept(resource, oldResource);
case DELETED:
throw new IllegalStateException("Should not be called with " + action);
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ public void start() throws OperatorException {
}

} catch (Exception e) {
log.error("Couldn't start informer for " + versionedFullResourceName() + " resources", e);
ReconcilerUtils.handleKubernetesClientException(e,
HasMetadata.getFullResourceName(informer.getApiTypeClass()));
throw e;
throw new OperatorException(
"Couldn't start informer for " + versionedFullResourceName() + " resources", e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ public class MySQLSchemaReconciler

static final Logger log = LoggerFactory.getLogger(MySQLSchemaReconciler.class);

public MySQLSchemaReconciler() {}


@Override
public UpdateControl<MySQLSchema> reconcile(MySQLSchema schema, Context<MySQLSchema> context) {
Expand Down