Copilot commented on code in PR #16231:
URL: https://github.com/apache/dubbo/pull/16231#discussion_r3144454935
##########
dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java:
##########
@@ -192,6 +200,23 @@ void test_getExtension_WithWrapper() {
assertEquals(echoCount2 + 1, Ext6Wrapper2.echoCount.get());
}
+ @Test
+ void test_getExtension_logsDebugWhenExtensionCreated() {
+ try (ExtensionLoaderTestContext<WrappedExt> testContext =
createExtensionLoaderTestContext(WrappedExt.class);
+ LogCollector logCollector =
LogCollector.attach(ExtensionLoader.class)) {
+ WrappedExt impl1 =
testContext.extensionLoader.getExtension("impl1");
Review Comment:
This test’s `LogCollector` only captures Log4j2 output, but
`ExtensionLoader` logs through Dubbo’s `LoggerFactory` adapter, which is
mutable global state and is set to non-log4j2 in other tests (e.g.,
`LoggerFactoryTest`, `FailsafeErrorTypeAwareLoggerTest`). That can make this
test order-dependent/flaky because the DEBUG line may never reach Log4j2.
Consider setting the logger adapter to `log4j2` for the duration of this test
(and restoring the previous adapter in `close()`/finally) before asserting on
captured Log4j2 events.
##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -988,6 +1005,16 @@ private Map<String, Class<?>> loadExtensionClasses()
throws InterruptedException
checkDestroyed();
cacheDefaultExtensionName();
+ long startNanos;
+ if (logger.isDebugEnabled()) {
+ startNanos = System.nanoTime();
+ logger.debug(
+ "Start loading extension classes, type={}, scopeModel={},
defaultName={}",
Review Comment:
`startNanos` is declared without initialization and is only assigned inside
the first `if (logger.isDebugEnabled())` block, but then is used in a
*separate* `if (logger.isDebugEnabled())` block. Java definite-assignment rules
will treat this as “might not have been initialized” (and it can also be
incorrect if `isDebugEnabled()` changes between calls). Consider capturing
`boolean debug = logger.isDebugEnabled();` once, initializing `startNanos`
(e.g., `0L`) and only computing/using timing when `debug` is true.
##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -794,13 +795,15 @@ private T createExtension(String name, boolean wrap) {
}
if (CollectionUtils.isNotEmpty(wrapperClassesList)) {
+ appliedWrapperClasses = new
ArrayList<>(wrapperClassesList.size());
for (Class<?> wrapperClass : wrapperClassesList) {
Wrapper wrapper =
wrapperClass.getAnnotation(Wrapper.class);
boolean match = (wrapper == null)
|| ((ArrayUtils.isEmpty(wrapper.matches())
||
ArrayUtils.contains(wrapper.matches(), name))
&&
!ArrayUtils.contains(wrapper.mismatches(), name));
if (match) {
+ appliedWrapperClasses.add(wrapperClass);
Review Comment:
`appliedWrapperClasses` is allocated and populated on every wrapped
extension creation, even when DEBUG logging is disabled. This adds extra
allocations and per-wrapper work on a hot path and contradicts the stated “no
performance impact when DEBUG is off”. Consider only tracking
`appliedWrapperClasses` when `logger.isDebugEnabled()` (e.g., keep it `null`
otherwise and guard `add` calls), or reuse the existing `wrapperClassesList`
only when needed for logging.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
