Copilot commented on code in PR #13884:
URL: https://github.com/apache/cloudstack/pull/13884#discussion_r4345588797
##########
engine/schema/src/test/java/com/cloud/storage/dao/StoragePoolTagsDaoImplTest.java:
##########
Review Comment:
`configDepot` is changed from a mocked `ConfigurationDao` to a plain
`ConfigDepotImpl` field, but it isn't initialized/mocked before use. As
written, `ConfigKey.init(configDepot)` and
`when(configDepot.getConfigStringValue(...))` will run with `configDepot ==
null` and fail with an NPE. Make `configDepot` a Mockito mock (e.g., annotate
it with `@Mock`, or assign `Mockito.mock(ConfigDepotImpl.class)` in `setup()`
before calling `ConfigKey.init(...)`).
##########
engine/components-api/src/main/java/com/cloud/network/security/SecurityGroupManager.java:
##########
@@ -33,6 +35,23 @@ public interface SecurityGroupManager {
public static final int TIME_BETWEEN_CLEANUPS = 60;
public static final int WORKER_THREAD_COUNT = 10;
+ ConfigKey<Integer> SecurityGroupWorkCleanupInterval = new
ConfigKey<>("Network", Integer.class,
+ "network.securitygroups.work.cleanup.interval", "120",
+ "Time interval (seconds) in which finished work is cleaned up from
the work table", true);
+
+ ConfigKey<Integer> SecurityGroupWorkerThreads = new ConfigKey<>("Network",
Integer.class,
+ "network.securitygroups.workers.pool.size", "50",
Review Comment:
The defaults introduced in the new `ConfigKey`s appear to change behavior vs
the prior implementation defaults. In `SecurityGroupManagerImpl`, the old code
fell back to `TIME_BETWEEN_CLEANUPS` (60s) and `WORKER_THREAD_COUNT` (10) when
config was unset; after this PR it becomes 120s and 50 threads. If the PR
intends to be transparent, update these `ConfigKey` default values to match the
previous fallbacks (or document/justify the intentional behavioral change).
##########
engine/components-api/src/main/java/com/cloud/network/NetworkStateListener.java:
##########
@@ -38,15 +36,16 @@
public class NetworkStateListener implements StateListener<State, Event,
Network> {
- @Inject
- private ConfigurationDao _configDao;
+ // Registered/owned by
com.cloud.server.ManagementServer#PublishResourceStateEvent; declared here too
so this
+ // module can read it without a reverse dependency on the server module.
+ private static final ConfigKey<Boolean> PublishResourceStateEvent = new
ConfigKey<>("Advanced", Boolean.class, "publish.resource.state.events", "true",
+ "enable or disable publishing of alert events on the event bus",
true);
Review Comment:
The description for `publish.resource.state.events` says it controls
publishing of 'alert events', but this listener publishes *resource state*
events. Update the description text to accurately reflect what is being
published (resource/network state events) to avoid confusing operators.
##########
engine/schema/src/test/java/com/cloud/storage/dao/StoragePoolTagsDaoImplTest.java:
##########
Review Comment:
`configDepot` is changed from a mocked `ConfigurationDao` to a plain
`ConfigDepotImpl` field, but it isn't initialized/mocked before use. As
written, `ConfigKey.init(configDepot)` and
`when(configDepot.getConfigStringValue(...))` will run with `configDepot ==
null` and fail with an NPE. Make `configDepot` a Mockito mock (e.g., annotate
it with `@Mock`, or assign `Mockito.mock(ConfigDepotImpl.class)` in `setup()`
before calling `ConfigKey.init(...)`).
--
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]
