Skip to content

Commit 7d90b79

Browse files
metacosmmetacosm
committed
refactor: clean some reported code smells (#1660)
1 parent 5d5dc00 commit 7d90b79

File tree

15 files changed

+94
-59
lines changed

15 files changed

+94
-59
lines changed

‎micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ public void controllerRegistered(Controller<?> controller) {
5555
gauges.put(controllerQueueName, controllerQueueSize);
5656
}
5757

58+
@Override
5859
public <T> T timeControllerExecution(ControllerExecution<T> execution) {
5960
final var name = execution.controllerName();
6061
final var execName = PREFIX + "controllers.execution." + execution.name();
@@ -65,7 +66,7 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
6566
"controller", name,
6667
"resource.name", resourceID.getName(),
6768
"resource.namespace", resourceID.getNamespace().orElse(""),
68-
"resource.scope", resourceID.getNamespace().isPresent() ? "namespace" : "cluster"));
69+
"resource.scope", getScope(resourceID)));
6970
final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY);
7071
if (gvk != null) {
7172
tags.addAll(List.of(
@@ -101,6 +102,11 @@ public <T> T timeControllerExecution(ControllerExecution<T> execution) {
101102
}
102103
}
103104

105+
private static String getScope(ResourceID resourceID) {
106+
return resourceID.getNamespace().isPresent() ? "namespace" : "cluster";
107+
}
108+
109+
@Override
104110
public void receivedEvent(Event event, Map<String, Object> metadata) {
105111
incrementCounter(event.getRelatedCustomResourceID(), "events.received",
106112
metadata,
@@ -151,6 +157,7 @@ public void reconciliationExecutionFinished(HasMetadata resource, Map<String, Ob
151157
controllerQueueSize.decrementAndGet();
152158
}
153159

160+
@Override
154161
public void failedReconciliation(HasMetadata resource, Exception exception,
155162
Map<String, Object> metadata) {
156163
var cause = exception.getCause();
@@ -164,6 +171,7 @@ public void failedReconciliation(HasMetadata resource, Exception exception,
164171
cause.getClass().getSimpleName());
165172
}
166173

174+
@Override
167175
public <T extends Map<?, ?>> T monitorSizeOf(T map, String name) {
168176
return registry.gaugeMapSize(PREFIX + name + ".size", Collections.emptyList(), map);
169177
}
@@ -183,7 +191,7 @@ private void incrementCounter(ResourceID id, String counterName, Map<String, Obj
183191
tags.addAll(List.of(
184192
"name", id.getName(),
185193
"namespace", id.getNamespace().orElse(""),
186-
"scope", id.getNamespace().isPresent() ? "namespace" : "cluster"));
194+
"scope", getScope(id)));
187195
if (additionalTagsNb > 0) {
188196
tags.addAll(List.of(additionalTags));
189197
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/CustomResourceUtils.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66
import io.fabric8.kubernetes.api.model.Namespaced;
77
import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition;
88

9-
public abstract class CustomResourceUtils {
9+
public class CustomResourceUtils {
10+
11+
private CustomResourceUtils() {}
1012

1113
/**
1214
* Applies internal validations that may not be handled by the fabric8 client.

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,12 @@
1313
import io.fabric8.kubernetes.client.KubernetesClient;
1414
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
1515
import io.fabric8.kubernetes.client.Version;
16-
import io.javaoperatorsdk.operator.api.config.*;
16+
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
17+
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceOverrider;
18+
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
19+
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
20+
import io.javaoperatorsdk.operator.api.config.ControllerConfigurationOverrider;
21+
import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager;
1722
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
1823
import io.javaoperatorsdk.operator.processing.Controller;
1924
import io.javaoperatorsdk.operator.processing.LifecycleAware;
@@ -112,9 +117,8 @@ public synchronized void start() {
112117
leaderElectionManager.start();
113118
started = true;
114119
} catch (Exception e) {
115-
log.error("Error starting operator", e);
116120
stop();
117-
throw e;
121+
throw new OperatorException("Error starting operator", e);
118122
}
119123
}
120124

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,11 @@ public static void handleKubernetesClientException(Exception e, String resourceT
132132

133133
if (e instanceof KubernetesClientException) {
134134
KubernetesClientException ke = (KubernetesClientException) e;
135-
if (404 == ke.getCode()) {
136-
// only throw MissingCRDException if the 404 error occurs on the target CRD
137-
if (resourceTypeName.equals(ke.getFullResourceName())
138-
|| matchesResourceType(resourceTypeName, ke)) {
139-
throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
140-
}
135+
// only throw MissingCRDException if the 404 error occurs on the target CRD
136+
if (404 == ke.getCode() &&
137+
(resourceTypeName.equals(ke.getFullResourceName())
138+
|| matchesResourceType(resourceTypeName, ke))) {
139+
throw new MissingCRDException(resourceTypeName, ke.getVersion(), e.getMessage(), e);
141140
}
142141
}
143142
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,9 @@ public ResourceEventFilter<P> getEventFilter() {
157157
return eventFilter;
158158
}
159159

160+
/**
161+
* @deprecated Use {@link OnAddFilter}, {@link OnUpdateFilter} and {@link GenericFilter} instead
162+
*/
160163
@Deprecated(forRemoval = true)
161164
protected void setEventFilter(ResourceEventFilter<P> eventFilter) {
162165
this.eventFilter = eventFilter;

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ public class Utils {
2424
private static final Logger log = LoggerFactory.getLogger(Utils.class);
2525
public static final String CHECK_CRD_ENV_KEY = "JAVA_OPERATOR_SDK_CHECK_CRD";
2626
public static final String DEBUG_THREAD_POOL_ENV_KEY = "JAVA_OPERATOR_SDK_DEBUG_THREAD_POOL";
27+
public static final String GENERIC_PARAMETER_TYPE_ERROR_PREFIX =
28+
"Couldn't retrieve generic parameter type from ";
2729

2830
/**
2931
* Attempts to load version information from a properties file produced at build time, currently
@@ -102,7 +104,7 @@ public static Class<?> getFirstTypeArgumentFromExtendedClass(Class<?> clazz) {
102104
Type type = clazz.getGenericSuperclass();
103105
return (Class<?>) ((ParameterizedType) type).getActualTypeArguments()[0];
104106
} catch (Exception e) {
105-
throw new RuntimeException("Couldn't retrieve generic parameter type from "
107+
throw new RuntimeException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
106108
+ clazz.getSimpleName()
107109
+ " because it doesn't extend a class that is parameterized with the type we want to retrieve",
108110
e);
@@ -118,49 +120,55 @@ public static Class<?> getTypeArgumentFromInterfaceByIndex(Class<?> clazz,
118120
Class<?> expectedImplementedInterface, int index) {
119121
if (expectedImplementedInterface.isAssignableFrom(clazz)) {
120122
final var genericInterfaces = clazz.getGenericInterfaces();
121-
Optional<? extends Class<?>> target = Optional.empty();
122-
if (genericInterfaces.length > 0) {
123-
// try to find the target interface among them
124-
target = Arrays.stream(genericInterfaces)
125-
.filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
126-
&& type instanceof ParameterizedType)
127-
.map(ParameterizedType.class::cast)
128-
.findFirst()
129-
.map(t -> {
130-
final Type argument = t.getActualTypeArguments()[index];
131-
if (argument instanceof Class) {
132-
return (Class<?>) argument;
133-
}
134-
// account for the case where the argument itself has parameters, which we will ignore
135-
// and just return the raw type
136-
if (argument instanceof ParameterizedType) {
137-
final var rawType = ((ParameterizedType) argument).getRawType();
138-
if (rawType instanceof Class) {
139-
return (Class<?>) rawType;
140-
}
141-
}
142-
throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
143-
+ expectedImplementedInterface.getSimpleName()
144-
+ " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
145-
+ argument);
146-
});
147-
}
148123

124+
var target = extractType(clazz, expectedImplementedInterface, index, genericInterfaces);
149125
if (target.isPresent()) {
150126
return target.get();
151127
}
152128

153-
// try the parent
129+
// try the parent if we didn't find a parameter type on the current class
154130
var parent = clazz.getSuperclass();
155131
if (!Object.class.equals(parent)) {
156132
return getTypeArgumentFromInterfaceByIndex(parent, expectedImplementedInterface, index);
157133
}
158134
}
159-
throw new IllegalArgumentException("Couldn't retrieve generic parameter type from "
135+
throw new IllegalArgumentException(GENERIC_PARAMETER_TYPE_ERROR_PREFIX
160136
+ clazz.getSimpleName() + " because it or its superclasses don't implement "
161137
+ expectedImplementedInterface.getSimpleName());
162138
}
163139

140+
private static Optional<? extends Class<?>> extractType(Class<?> clazz,
141+
Class<?> expectedImplementedInterface, int index, Type[] genericInterfaces) {
142+
Optional<? extends Class<?>> target = Optional.empty();
143+
if (genericInterfaces.length > 0) {
144+
// try to find the target interface among them
145+
target = Arrays.stream(genericInterfaces)
146+
.filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName())
147+
&& type instanceof ParameterizedType)
148+
.map(ParameterizedType.class::cast)
149+
.findFirst()
150+
.map(t -> {
151+
final Type argument = t.getActualTypeArguments()[index];
152+
if (argument instanceof Class) {
153+
return (Class<?>) argument;
154+
}
155+
// account for the case where the argument itself has parameters, which we will ignore
156+
// and just return the raw type
157+
if (argument instanceof ParameterizedType) {
158+
final var rawType = ((ParameterizedType) argument).getRawType();
159+
if (rawType instanceof Class) {
160+
return (Class<?>) rawType;
161+
}
162+
}
163+
throw new IllegalArgumentException(clazz.getSimpleName() + " implements "
164+
+ expectedImplementedInterface.getSimpleName()
165+
+ " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "
166+
+ argument);
167+
});
168+
}
169+
return target;
170+
}
171+
164172
public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> clazz,
165173
Class<?> expectedImplementedInterface) {
166174
// first check super class if it exists
@@ -183,7 +191,7 @@ public static Class<?> getFirstTypeArgumentFromSuperClassOrInterface(Class<?> cl
183191
return getFirstTypeArgumentFromInterface(clazz, expectedImplementedInterface);
184192
} catch (Exception e) {
185193
throw new OperatorException(
186-
"Couldn't retrieve generic parameter type from " + clazz.getSimpleName(), e);
194+
GENERIC_PARAMETER_TYPE_ERROR_PREFIX + clazz.getSimpleName(), e);
187195
}
188196
}
189197

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ default void controllerRegistered(Controller<?> controller) {}
3535
*/
3636
default void receivedEvent(Event event, Map<String, Object> metadata) {}
3737

38+
/**
39+
* @deprecated Use {@link #reconcileCustomResource(HasMetadata, RetryInfo, Map)} instead
40+
*/
3841
@Deprecated(forRemoval = true)
3942
default void reconcileCustomResource(ResourceID resourceID, RetryInfo retryInfo,
4043
Map<String, Object> metadata) {}
@@ -51,6 +54,9 @@ default void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfo,
5154
reconcileCustomResource(ResourceID.fromResource(resource), retryInfo, metadata);
5255
}
5356

57+
/**
58+
* @deprecated Use {@link #failedReconciliation(HasMetadata, Exception, Map)} instead
59+
*/
5460
@Deprecated(forRemoval = true)
5561
default void failedReconciliation(ResourceID resourceID, Exception exception,
5662
Map<String, Object> metadata) {}
@@ -102,6 +108,9 @@ default void finishedReconciliation(ResourceID resourceID) {
102108
finishedReconciliation(resourceID, Collections.emptyMap());
103109
}
104110

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

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultManagedWorkflow.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,18 @@ public class DefaultManagedWorkflow<P extends HasMetadata> implements ManagedWor
2222

2323
private final Set<String> topLevelResources;
2424
private final Set<String> bottomLevelResources;
25-
private final List<DependentResourceSpec<?, ?>> orderedSpecs;
25+
private final List<DependentResourceSpec> orderedSpecs;
2626
private final boolean hasCleaner;
2727

28-
protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
28+
protected DefaultManagedWorkflow(List<DependentResourceSpec> orderedSpecs,
2929
boolean hasCleaner) {
3030
this.hasCleaner = hasCleaner;
3131
topLevelResources = new HashSet<>(orderedSpecs.size());
3232
bottomLevelResources = orderedSpecs.stream()
3333
.map(DependentResourceSpec::getName)
3434
.collect(Collectors.toSet());
3535
this.orderedSpecs = orderedSpecs;
36-
orderedSpecs.forEach(spec -> {
36+
for (DependentResourceSpec<?, ?> spec : orderedSpecs) {
3737
// add cycle detection?
3838
if (spec.getDependsOn().isEmpty()) {
3939
topLevelResources.add(spec.getName());
@@ -42,12 +42,12 @@ protected DefaultManagedWorkflow(List<DependentResourceSpec<?, ?>> orderedSpecs,
4242
bottomLevelResources.remove(dependsOn);
4343
}
4444
}
45-
});
45+
}
4646
}
4747

4848
@Override
4949
@SuppressWarnings("unused")
50-
public List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
50+
public List<DependentResourceSpec> getOrderedSpecs() {
5151
return orderedSpecs;
5252
}
5353

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public DependentResourceNode(String name, Condition<R, P> reconcilePrecondition,
3232
this.dependentResource = dependentResource;
3333
}
3434

35-
public List<? extends DependentResourceNode> getDependsOn() {
35+
public List<DependentResourceNode> getDependsOn() {
3636
return dependsOn;
3737
}
3838

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflow.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010

1111
public interface ManagedWorkflow<P extends HasMetadata> {
1212

13-
@SuppressWarnings("unused")
14-
default List<DependentResourceSpec<?, ?>> getOrderedSpecs() {
13+
@SuppressWarnings({"unused", "rawtypes"})
14+
default List<DependentResourceSpec> getOrderedSpecs() {
1515
return Collections.emptyList();
1616
}
1717

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupport.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,11 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
6464
* @return top-bottom ordered resources that can be added safely to workflow
6565
* @throws OperatorException if there is a cycle in the dependencies
6666
*/
67-
private List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
67+
private List<DependentResourceSpec> orderAndDetectCycles(
6868
List<DependentResourceSpec> dependentResourceSpecs, boolean[] cleanerHolder) {
6969

7070
final var drInfosByName = createDRInfos(dependentResourceSpecs);
71-
final var orderedSpecs =
72-
new ArrayList<DependentResourceSpec<?, ?>>(dependentResourceSpecs.size());
71+
final var orderedSpecs = new ArrayList<DependentResourceSpec>(dependentResourceSpecs.size());
7372
final var alreadyVisited = new HashSet<String>();
7473
var toVisit = getTopDependentResources(dependentResourceSpecs);
7574

@@ -108,7 +107,7 @@ <P extends HasMetadata> DefaultManagedWorkflow<P> createAsDefault(
108107
* @return top-bottom ordered resources that can be added safely to workflow
109108
* @throws OperatorException if there is a cycle in the dependencies
110109
*/
111-
public List<DependentResourceSpec<?, ?>> orderAndDetectCycles(
110+
public List<DependentResourceSpec> orderAndDetectCycles(
112111
List<DependentResourceSpec> dependentResourceSpecs) {
113112
return orderAndDetectCycles(dependentResourceSpecs, null);
114113
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ResourceState.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,9 @@ public void unMarkEventReceived() {
106106
throw new IllegalStateException("Cannot unmark processed marked for deletion.");
107107
case DELETE_EVENT_PRESENT:
108108
throw new IllegalStateException("Cannot unmark delete event.");
109+
case NO_EVENT_PRESENT:
110+
// do nothing
111+
break;
109112
}
110113
}
111114
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ private boolean isAcceptedByFilters(ResourceAction action, T resource, T oldReso
9494
return onAddFilter == null || onAddFilter.accept(resource);
9595
case UPDATED:
9696
return onUpdateFilter.accept(resource, oldResource);
97+
case DELETED:
98+
throw new IllegalStateException("Should not be called with " + action);
9799
}
98100
return true;
99101
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerWrapper.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,10 @@ public void start() throws OperatorException {
9494
}
9595

9696
} catch (Exception e) {
97-
log.error("Couldn't start informer for " + versionedFullResourceName() + " resources", e);
9897
ReconcilerUtils.handleKubernetesClientException(e,
9998
HasMetadata.getFullResourceName(informer.getApiTypeClass()));
100-
throw e;
99+
throw new OperatorException(
100+
"Couldn't start informer for " + versionedFullResourceName() + " resources", e);
101101
}
102102
}
103103

‎sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@ public class MySQLSchemaReconciler
3030

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

33-
public MySQLSchemaReconciler() {}
34-
3533

3634
@Override
3735
public UpdateControl<MySQLSchema> reconcile(MySQLSchema schema, Context<MySQLSchema> context) {

0 commit comments

Comments
 (0)