You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "Hyeon-moGu (via GitHub)" <gi...@apache.org> on 2026/03/01 17:38:05 UTC
[PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Hyeon-moGu opened a new pull request, #13718:
URL: https://github.com/apache/skywalking/pull/13718
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #13709 .
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
### [Analyzer]
**General Refactoring (`agent-analyzer`, `log-analyzer`):**
- Systematically eliminated `Whitebox` usages across various analyzer tests.
- Migrated internal state manipulations to standard Java Reflection API (`Field.setAccessible()` etc.), ensuring proper checked exception handling to prevent polluting test method signatures.
**`AnalyzerTest` (meter-analyzer):**
- Replaced `Whitebox` with `MockedStatic` (using Mockito) to safely inject and mock the `MetricsStreamProcessor` singleton.
- Added explicit `.close()` calls in the `@AfterEach` (or `tearDownEach`) lifecycle methods to properly release the static mocks and prevent resource leaks or interference between tests.
**`K8sTagTest` (meter-analyzer):**
- Since `KubernetesPods` and `KubernetesServices` are implemented as Enum singletons, mocking the entire objects was problematic. To resolve this:
- Handled the internal lazy-loading `LoadingCache` instances by injecting Mock objects directly into these fields via standard reflection.
- Fixed potential `NullPointerException`s during cache misses by explicitly stubbing the mocked caches to return `Optional.empty()` or empty collections for non-matching keys, ensuring better test isolation and stability.
### [Server-Cluster-Plugin]
- Replaced all `Whitebox.setInternalState()` and `getInternalState()` usages with standard `java.lang.reflect.Field` to inject internal dependencies (e.g., `client`, `config`, `loadedProvider`, `healthChecker`) into Coordinators and Providers across all 5 cluster plugins (Zookeeper, Nacos, Etcd, Kubernetes, Consul).
- **Handled Checked Exceptions Safely:** Since standard Reflection APIs strictly throw checked exceptions (`NoSuchFieldException`, `IllegalAccessException`), explicitly wrapped them in `RuntimeException` within `try-catch` blocks. This successfully removed the PowerMock dependency while preserving the existing test lifecycle method signatures (like `@BeforeEach` or `@Test`) without adding unnecessary `throws` declarations.
### [Receiver & Fetcher Plugins]
**General Refactoring (`server-receiver-plugin`, `server-fetcher-plugin`):**
- Systematically removed `Whitebox` dependencies and migrated to standard Java Reflection API.
- Centralized reflection logic within `try-catch` blocks to wrap checked exceptions into `RuntimeException`, ensuring no changes were required to existing test method signatures.
`ZabbixMetricsTest` & `ZabbixBaseTest`:
- Static Mock Lifecycle Management: Replaced manual `Whitebox` field manipulation of the `MetricsStreamProcessor` singleton with `MockedStatic`.
- Implemented strict `.close()` logic within the `@AfterEach` lifecycle to prevent memory leaks and resource conflicts, which is particularly critical for stability in newer JVM environments.
- Resolved a legacy `NullPointerException` by combining `MockedStatic` with a `Mockito.spy()` of the actual processor instance, ensuring a safe and stateful mock return.
`TelegrafMetricsTest`:
- Eliminated Redundant Mocks: Removed unused `CoreModule` and `MetricsStreamProcessor` stubbings.
- Benefit: Removing these dead mock configurations simplifies the test setup, adheres to the minimal mocking principle, and ensures full compatibility with modern Mockito strictness.
`ClusterManagerMetricsAdapterTest`:
- Static State Isolation: Addressed potential flaky test behavior by explicitly clearing shared static maps (e.g., `initialized` flag in `FieldsHelper`) during `@BeforeEach`.
- This ensures complete test isolation by preventing state leakage between test cases, leading to a more deterministic and reliable test suite.
`CiliumNodeManagerTest` & Others:
- Successfully migrated internal state injections(remoteInstances and allNodes) to standard reflection, maintaining the original testing intent while removing the PowerMock requirement.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-4009307791
A lot of conflicts, as we have MAL/LAL v2, you need to adopt on latest codes.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-3989110257
Please follow copolit review(check whether they are reasonable)
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#discussion_r2870034447
##########
docs/en/changes/changes.md:
##########
@@ -66,7 +66,7 @@
| Schedulers + others | ~24 | ~24 | Mostly unchanged |
| **Total (OAP threads)** | **150+** | **~72** | **~50% reduction, stable in high payload.** |
-* Replace PowerMock Whitebox with standard Java Reflection in `server-library`, `server-core`, and `server-configuration` to support JDK 25+.
+* Replace PowerMock Whitebox with standard Java Reflection in `server-library`, `server-core`, `server-configuration`, `analyzer`, `server-cluster-plugin`, `server-receiver-plugin`, `server-fetcher-plugin` to support JDK 25+.
Review Comment:
There is an extra space before `server-cluster-plugin` in the changelog entry: `` `analyzer`, `server-cluster-plugin` `` has two spaces instead of one after the comma.
```suggestion
* Replace PowerMock Whitebox with standard Java Reflection in `server-library`, `server-core`, `server-configuration`, `analyzer`, `server-cluster-plugin`, `server-receiver-plugin`, `server-fetcher-plugin` to support JDK 25+.
```
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/K8sTagTest.java:
##########
@@ -237,29 +240,61 @@ public static Collection<Object[]> data() {
@SneakyThrows
@BeforeEach
public void setup() {
- Whitebox.setInternalState(KubernetesServices.class, "INSTANCE",
- Mockito.mock(KubernetesServices.class)
- );
- Whitebox.setInternalState(KubernetesPods.class, "INSTANCE",
- Mockito.mock(KubernetesPods.class)
- );
+ LoadingCache<KubernetesServices, Object> mockServicesCache = mock(LoadingCache.class);
+ LoadingCache<ObjectID, Object> mockServiceByIDCache = mock(LoadingCache.class);
+
+ Field servicesField = KubernetesServices.class.getDeclaredField("services");
+ servicesField.setAccessible(true);
+ servicesField.set(KubernetesServices.INSTANCE, mockServicesCache);
+
+ Field serviceByIDField = KubernetesServices.class.getDeclaredField("serviceByID");
+ serviceByIDField.setAccessible(true);
+ serviceByIDField.set(KubernetesServices.INSTANCE, mockServiceByIDCache);
+
+ LoadingCache<String, Object> mockPodByIPCache = mock(LoadingCache.class);
+ LoadingCache<ObjectID, Object> mockPodByObjectIDCache = mock(LoadingCache.class);
+
+ Field podByIPField = KubernetesPods.class.getDeclaredField("podByIP");
+ podByIPField.setAccessible(true);
+ podByIPField.set(KubernetesPods.INSTANCE, mockPodByIPCache);
+
+ Field podByObjectIDField = KubernetesPods.class.getDeclaredField("podByObjectID");
+ podByObjectIDField.setAccessible(true);
+ podByObjectIDField.set(KubernetesPods.INSTANCE, mockPodByObjectIDCache);
- when(KubernetesServices.INSTANCE.list()).thenReturn(ImmutableList.of(
+ try {
+ when(mockServicesCache.get(any())).thenReturn(ImmutableList.of());
+ when(mockServiceByIDCache.get(any())).thenReturn(Optional.empty());
+ when(mockPodByIPCache.get(any())).thenReturn(Optional.empty());
+ when(mockPodByObjectIDCache.get(any())).thenReturn(Optional.empty());
+ } catch (Exception e) {
+ // ignore
+ }
+
+ when(mockServicesCache.get(KubernetesServices.INSTANCE)).thenReturn(ImmutableList.of(
mockService("nginx-service", "default", of("run", "nginx"), "2.2.2.1"),
mockService("kube-state-metrics", "kube-system", of("run", "kube-state-metrics"), "2.2.2.2")));
ImmutableList.of(
mockService("nginx-service", "default", of("run", "nginx"), "2.2.2.1"),
mockService("kube-state-metrics", "kube-system", of("run", "kube-state-metrics"), "2.2.2.2"))
- .forEach(svc ->
- when(KubernetesServices.INSTANCE.findByID(ObjectID.builder().namespace(svc.getMetadata().getNamespace()).name(svc.getMetadata().getName()).build()))
- .thenReturn(Optional.of(svc))
- );
+ .forEach(svc -> {
+ try {
+ when(mockServiceByIDCache.get(ObjectID.builder().namespace(svc.getMetadata().getNamespace()).name(svc.getMetadata().getName()).build()))
+ .thenReturn(Optional.of(svc));
+ } catch (Exception e) {
+ // ignore
+ }
+ });
ImmutableList.of(
mockPod("my-nginx-5dc4865748-mbczh", "default", of("run", "nginx"), "1.1.1.1"),
mockPod("kube-state-metrics-6f979fd498-z7xwx", "kube-system", of("run", "kube-state-metrics"), "1.1.1.2"))
.forEach(pod -> {
- when(KubernetesPods.INSTANCE.findByIP(pod.getStatus().getPodIP())).thenReturn(Optional.of(pod));
- when(KubernetesPods.INSTANCE.findByObjectID(ObjectID.builder().name(pod.getMetadata().getName()).namespace(pod.getMetadata().getNamespace()).build())).thenReturn(Optional.of(pod));
+ try {
+ when(mockPodByIPCache.get(pod.getStatus().getPodIP())).thenReturn(Optional.of(pod));
+ when(mockPodByObjectIDCache.get(ObjectID.builder().name(pod.getMetadata().getName()).namespace(pod.getMetadata().getNamespace()).build())).thenReturn(Optional.of(pod));
+ } catch (Exception e) {
+ // ignore
+ }
});
Review Comment:
Several `catch (Exception e) { // ignore }` blocks silently swallow exceptions from `when(mockCache.get(...)).thenReturn(...)` calls. While `LoadingCache.get()` declares a checked `ExecutionException`, it will never actually be thrown when stubbing with Mockito — the `when()` call is only setting up expectations, not actually invoking the method. However, silently catching and ignoring exceptions in test code is dangerous: if a real exception occurs (e.g., `UnstubbedMethodInvocationException`, or wrong argument type), the test setup would fail silently, leading to test failures with misleading error messages further downstream rather than at the point of failure. Consider either propagating the exception or at minimum logging/rethrowing it.
##########
oap-server/server-cluster-plugin/cluster-consul-plugin/src/test/java/org/apache/skywalking/oap/server/cluster/plugin/consul/ConsulCoordinatorTest.java:
##########
@@ -32,7 +32,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
Review Comment:
The `import java.lang.reflect.Field;` statement is placed between `org.*` imports and the existing `java.util.*` imports block, breaking the established import ordering convention used throughout the codebase (where `java.*` imports are grouped separately from third-party imports). The `java.lang.reflect.Field` import should be placed in the `java.*` import section rather than after `org.mockito.*` imports.
```suggestion
```
##########
oap-server/server-receiver-plugin/skywalking-zabbix-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/zabbix/provider/ZabbixMetricsTest.java:
##########
@@ -45,7 +45,9 @@
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
+import org.junit.jupiter.api.AfterEach;
+import org.mockito.MockedStatic;
Review Comment:
The newly added imports (`import java.lang.reflect.Field`, `import org.junit.jupiter.api.AfterEach`, `import org.mockito.MockedStatic`) are placed in the middle of other grouped imports, breaking the import ordering convention. Specifically, `import java.lang.reflect.Field` should be in the `java.*` section, `import org.junit.jupiter.api.AfterEach` should be adjacent to the other `org.junit.jupiter.api.*` imports, and `import org.mockito.MockedStatic` should be adjacent to the other `org.mockito.*` imports.
##########
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/envoy/ClusterManagerMetricsAdapterTest.java:
##########
@@ -41,8 +42,11 @@ public class ClusterManagerMetricsAdapterTest {
@SneakyThrows
@BeforeEach
- public void setUp() {
- Whitebox.setInternalState(FieldsHelper.forClass(this.getClass()), "initialized", false);
+ public void setUp() throws Exception {
Review Comment:
The `setUp()` method has both `@SneakyThrows` (from Lombok) and `throws Exception` declared on the same method signature. These are redundant — `@SneakyThrows` suppresses the need to declare checked exceptions, so the `throws Exception` declaration is unnecessary. However, the original method already had `@SneakyThrows` before this change, so the only new addition here is `throws Exception`, which is the redundant part.
```suggestion
public void setUp() {
```
##########
oap-server/server-cluster-plugin/cluster-zookeeper-plugin/src/test/java/org/apache/skywalking/oap/server/cluster/plugin/zookeeper/ZookeeperCoordinatorTest.java:
##########
@@ -30,7 +30,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
Review Comment:
The `import java.lang.reflect.Field;` statement is placed after `org.mockito.*` imports, breaking the established import ordering convention where `java.*` imports are grouped together (as seen in the project's other production and test files, e.g., `ConsulCoordinator.java`). The new import should be placed within or adjacent to the existing `java.util.*` import block.
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/AnalyzerTest.java:
##########
@@ -86,6 +88,13 @@ public static void tearDown() {
MeterEntity.setNamingControl(null);
}
+ @org.junit.jupiter.api.AfterEach
+ public void tearDownEach() {
Review Comment:
The `@AfterEach` annotation is used as a fully-qualified annotation (`@org.junit.jupiter.api.AfterEach`) rather than using the existing import convention. The `@BeforeEach` is imported and used without full qualification in the same file. The `@AfterEach` import should be added and used consistently without full qualification.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "Hyeon-moGu (via GitHub)" <gi...@apache.org>.
Hyeon-moGu commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-3990122379
I've applied the reasonable Copilot suggestions, including fixing the import order and removing redundant exception declarations.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "Hyeon-moGu (via GitHub)" <gi...@apache.org>.
Hyeon-moGu commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-3981770315
> Could you remove the changes of log-analyzer and meter-analyzer? I am working on to big changes to replace them, ref to #13716 Most likely, everything in there will be changed.
Updated as requested to remove `log-analyzer` and `meter-analyzer`. Thanks!
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #13718: Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins.
URL: https://github.com/apache/skywalking/pull/13718
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-3981345811
Could you remove the changes of log-analyzer and meter-analyzer? I am working on to big changes to replace them, ref to https://github.com/apache/skywalking/discussions/13716
Most likely, everything in there will be changed.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "Hyeon-moGu (via GitHub)" <gi...@apache.org>.
Hyeon-moGu commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-4009778408
Got it, thanks for the heads-up
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-4009321900
Powermock should have been replaced out by me.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor: Replace Whitebox with standard reflection in Analyzer, Server-Cluster-Plugin and Receiver & Fetcher Plugins. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#issuecomment-4009342451
I checked.
> The groovy-replace branch has already replaced all Whitebox usages with ReflectUtil. PR #13718 is no longer required — the work it proposes has already been done on this branch.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org