Copilot commented on code in PR #7621:
URL: https://github.com/apache/shenyu/pull/7621#discussion_r4266385234
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/IngressControllerConfiguration.java:
##########
@@ -83,26 +87,52 @@ public class IngressControllerConfiguration {
public ControllerManager controllerManager(final SharedInformerFactory
sharedInformerFactory,
@Qualifier("ingress-controller") final Controller ingressController,
@Qualifier("endpoints-controller") final Controller endpointsController) {
- ControllerManager controllerManager = new
ControllerManager(sharedInformerFactory, ingressController,
endpointsController);
- Executors.newSingleThreadExecutor().submit(controllerManager);
- return controllerManager;
+ return new ControllerManager(sharedInformerFactory, ingressController,
endpointsController);
+ }
+
+ /**
+ * Controller executor service.
+ *
+ * @return controller executor service
+ */
+ @Bean(destroyMethod = "shutdown")
+ public ExecutorService ingressControllerExecutorService() {
+ return Executors.newSingleThreadExecutor(runnable -> {
+ Thread thread = new Thread(runnable,
"shenyu-k8s-ingress-controller");
+ thread.setDaemon(true);
Review Comment:
Marking the controller thread as daemon means it can be terminated abruptly
on JVM shutdown (potentially skipping orderly shutdown paths depending on how
the application exits). Since you already manage lifecycle (`SmartLifecycle`)
and executor shutdown, consider making the thread non-daemon and relying on
Spring’s stop/destroy callbacks for graceful termination, or document why
daemon behavior is required here.
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/IngressControllerConfiguration.java:
##########
@@ -133,15 +163,17 @@ public IngressReconciler ingressReconciler(final
SharedIndexInformer<V1Ingress>
*
* @param sharedInformerFactory sharedInformerFactory
* @param endpointsReconciler endpointsReconciler
+ * @param properties controller properties
* @return Endpoints Controller
*/
@Bean("endpoints-controller")
- public Controller endpointsController(final SharedInformerFactory
sharedInformerFactory, final EndpointsReconciler endpointsReconciler) {
+ public Controller endpointsController(final SharedInformerFactory
sharedInformerFactory, final EndpointsReconciler endpointsReconciler,
+ final
ShenyuKubernetesControllerProperties properties) {
DefaultControllerBuilder builder =
ControllerBuilder.defaultBuilder(sharedInformerFactory);
builder = builder.watch(q ->
ControllerBuilder.controllerWatchBuilder(V1Endpoints.class, q)
- .withResyncPeriod(Duration.ofMinutes(1))
+ .withResyncPeriod(properties.getResyncPeriod())
.build());
- builder.withWorkerCount(2);
+ builder.withWorkerCount(properties.getWorkerCount());
return
builder.withReconciler(endpointsReconciler).withName("ingressController").build();
Review Comment:
`endpointsController(...)` is built with the name `"ingressController"`,
which duplicates the ingress controller name and makes logs/metrics/debugging
ambiguous (and may conflict if the underlying controller framework expects
unique names). Use a distinct controller name (e.g., `"endpointsController"`),
consistent with the bean’s purpose.
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/ShenyuKubernetesControllerProperties.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.springboot.starter.k8s;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+import java.time.Duration;
+
+/**
+ * Configuration properties for Kubernetes ingress and endpoints controllers.
+ */
+@ConfigurationProperties(prefix = "shenyu.k8s.controller")
+public class ShenyuKubernetesControllerProperties {
+
+ /**
+ * Number of workers used by each controller. Values greater than one rely
on the
+ * ingress, selector, secret and service relation caches remaining
thread-safe.
+ */
+ private int workerCount = 2;
Review Comment:
The setters accept invalid values (e.g., `workerCount <= 0`, negative
`resyncPeriod`), which can lead to runtime failures or undefined
controller/informer behavior. Add bean validation constraints (e.g., `@Min(1)`
for `workerCount` and a non-negative/positive duration constraint) and ensure
validation is enabled for this `@ConfigurationProperties` type (e.g., via
`@Validated`), or enforce invariants in setters with clear
`IllegalArgumentException`s.
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/ShenyuKubernetesControllerProperties.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.springboot.starter.k8s;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+import java.time.Duration;
+
+/**
+ * Configuration properties for Kubernetes ingress and endpoints controllers.
+ */
+@ConfigurationProperties(prefix = "shenyu.k8s.controller")
+public class ShenyuKubernetesControllerProperties {
+
+ /**
+ * Number of workers used by each controller. Values greater than one rely
on the
+ * ingress, selector, secret and service relation caches remaining
thread-safe.
+ */
+ private int workerCount = 2;
+
+ /**
+ * Period between full informer resynchronizations.
+ */
+ private Duration resyncPeriod = Duration.ofMinutes(1);
Review Comment:
The setters accept invalid values (e.g., `workerCount <= 0`, negative
`resyncPeriod`), which can lead to runtime failures or undefined
controller/informer behavior. Add bean validation constraints (e.g., `@Min(1)`
for `workerCount` and a non-negative/positive duration constraint) and ensure
validation is enabled for this `@ConfigurationProperties` type (e.g., via
`@Validated`), or enforce invariants in setters with clear
`IllegalArgumentException`s.
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/ShenyuKubernetesControllerProperties.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.springboot.starter.k8s;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+import java.time.Duration;
+
+/**
+ * Configuration properties for Kubernetes ingress and endpoints controllers.
+ */
+@ConfigurationProperties(prefix = "shenyu.k8s.controller")
+public class ShenyuKubernetesControllerProperties {
+
+ /**
+ * Number of workers used by each controller. Values greater than one rely
on the
+ * ingress, selector, secret and service relation caches remaining
thread-safe.
+ */
+ private int workerCount = 2;
+
+ /**
+ * Period between full informer resynchronizations.
+ */
+ private Duration resyncPeriod = Duration.ofMinutes(1);
+
+ /**
+ * Get worker count.
+ *
+ * @return worker count
+ */
+ public int getWorkerCount() {
+ return workerCount;
+ }
+
+ /**
+ * Set worker count.
+ *
+ * @param workerCount worker count
+ */
+ public void setWorkerCount(final int workerCount) {
+ this.workerCount = workerCount;
+ }
Review Comment:
The setters accept invalid values (e.g., `workerCount <= 0`, negative
`resyncPeriod`), which can lead to runtime failures or undefined
controller/informer behavior. Add bean validation constraints (e.g., `@Min(1)`
for `workerCount` and a non-negative/positive duration constraint) and ensure
validation is enabled for this `@ConfigurationProperties` type (e.g., via
`@Validated`), or enforce invariants in setters with clear
`IllegalArgumentException`s.
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-k8s/src/main/java/org/apache/shenyu/springboot/starter/k8s/ShenyuKubernetesControllerProperties.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.springboot.starter.k8s;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+import java.time.Duration;
+
+/**
+ * Configuration properties for Kubernetes ingress and endpoints controllers.
+ */
+@ConfigurationProperties(prefix = "shenyu.k8s.controller")
+public class ShenyuKubernetesControllerProperties {
+
+ /**
+ * Number of workers used by each controller. Values greater than one rely
on the
+ * ingress, selector, secret and service relation caches remaining
thread-safe.
+ */
+ private int workerCount = 2;
+
+ /**
+ * Period between full informer resynchronizations.
+ */
+ private Duration resyncPeriod = Duration.ofMinutes(1);
+
+ /**
+ * Get worker count.
+ *
+ * @return worker count
+ */
+ public int getWorkerCount() {
+ return workerCount;
+ }
+
+ /**
+ * Set worker count.
+ *
+ * @param workerCount worker count
+ */
+ public void setWorkerCount(final int workerCount) {
+ this.workerCount = workerCount;
+ }
+
+ /**
+ * Get resync period.
+ *
+ * @return resync period
+ */
+ public Duration getResyncPeriod() {
+ return resyncPeriod;
+ }
+
+ /**
+ * Set resync period.
+ *
+ * @param resyncPeriod resync period
+ */
+ public void setResyncPeriod(final Duration resyncPeriod) {
+ this.resyncPeriod = resyncPeriod;
+ }
Review Comment:
The setters accept invalid values (e.g., `workerCount <= 0`, negative
`resyncPeriod`), which can lead to runtime failures or undefined
controller/informer behavior. Add bean validation constraints (e.g., `@Min(1)`
for `workerCount` and a non-negative/positive duration constraint) and ensure
validation is enabled for this `@ConfigurationProperties` type (e.g., via
`@Validated`), or enforce invariants in setters with clear
`IllegalArgumentException`s.
--
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]
