Copilot commented on code in PR #16231:
URL: https://github.com/apache/dubbo/pull/16231#discussion_r3136081970
##########
dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java:
##########
@@ -882,4 +907,88 @@ public int getPriority() {
return MAX_PRIORITY;
}
}
+
+ private <T> ExtensionLoaderTestContext<T>
createExtensionLoaderTestContext(Class<T> type) {
+ FrameworkModel frameworkModel = new FrameworkModel();
+ ApplicationModel applicationModel = frameworkModel.newApplication();
+ return new ExtensionLoaderTestContext<>(
+ frameworkModel,
applicationModel.getExtensionDirector().getExtensionLoader(type));
+ }
+
+ private static final class ExtensionLoaderTestContext<T> implements
AutoCloseable {
+ private final FrameworkModel frameworkModel;
+ private final ExtensionLoader<T> extensionLoader;
+
+ private ExtensionLoaderTestContext(FrameworkModel frameworkModel,
ExtensionLoader<T> extensionLoader) {
+ this.frameworkModel = frameworkModel;
+ this.extensionLoader = extensionLoader;
+ }
+
+ @Override
+ public void close() {
+ frameworkModel.destroy();
+ }
+ }
+
+ private static final class LogCollector implements AutoCloseable {
+ private final LoggerContext context;
+ private final Configuration configuration;
+ private final String loggerName;
+ private final LoggerConfig loggerConfig;
+ private final TestAppender appender;
+
+ private LogCollector(
+ LoggerContext context,
+ Configuration configuration,
+ String loggerName,
+ LoggerConfig loggerConfig,
+ TestAppender appender) {
+ this.context = context;
+ this.configuration = configuration;
+ this.loggerName = loggerName;
+ this.loggerConfig = loggerConfig;
+ this.appender = appender;
+ }
+
+ static LogCollector attach(Class<?> loggerType) {
+ LoggerContext context = LoggerContext.getContext(false);
+ Configuration configuration = context.getConfiguration();
+ String loggerName = loggerType.getName();
+ TestAppender appender = new TestAppender("test-appender-" +
loggerType.getSimpleName());
+ appender.start();
+ configuration.addAppender(appender);
+
+ LoggerConfig loggerConfig = new LoggerConfig(loggerName,
org.apache.logging.log4j.Level.DEBUG, false);
+ loggerConfig.addAppender(appender,
org.apache.logging.log4j.Level.DEBUG, null);
+ configuration.addLogger(loggerName, loggerConfig);
+ context.updateLoggers();
+ return new LogCollector(context, configuration, loggerName,
loggerConfig, appender);
Review Comment:
`LogCollector.attach` installs a brand-new `LoggerConfig` for `loggerName`
and `close()` removes it, which can clobber any pre-existing logger
configuration for that name and cause cross-test side effects. Consider
capturing the previous `LoggerConfig` (or using
`configuration.getLoggerConfig(loggerName)` and only adding/removing an
appender) and restoring the original level/additivity on close.
##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -988,6 +1006,15 @@ private Map<String, Class<?>> loadExtensionClasses()
throws InterruptedException
checkDestroyed();
cacheDefaultExtensionName();
+ long startNanos = System.nanoTime();
+ if (logger.isDebugEnabled()) {
+ logger.debug(
+ "Start loading extension classes, type={}, scopeModel={},
defaultName={}",
+ type.getName(),
+ scopeModel,
+ cachedDefaultName);
+ }
Review Comment:
`loadExtensionClasses()` captures `startNanos` unconditionally. That adds
overhead even when DEBUG is disabled and contradicts the PR description of “no
performance impact”. Consider only calling `System.nanoTime()` when
`logger.isDebugEnabled()` (e.g., compute `startNanos` inside the debug block or
use a sentinel).
##########
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");
+
+ assertNotNull(impl1);
+ assertTrue(logCollector.contains("Loaded extension instance,
type=" + WrappedExt.class.getName()));
+ assertTrue(logCollector.contains("name=impl1"));
+ assertTrue(logCollector.contains("instanceClass=" +
Ext6Wrapper1.class.getName())
+ || logCollector.contains("instanceClass=" +
Ext6Wrapper2.class.getName()));
+ assertTrue(logCollector.contains("wrapperClasses=["));
+ assertTrue(logCollector.contains(Ext6Wrapper1.class.getName()));
+ assertTrue(logCollector.contains(Ext6Wrapper2.class.getName()));
Review Comment:
These assertions can be satisfied by *other* DEBUG log lines (e.g.,
class-loading logs) rather than the specific "Loaded extension instance" line,
so the test may pass even if the instance log doesn’t include wrapper info.
Consider locating the single log message that contains "Loaded extension
instance" and asserting all expected substrings
(name/instanceClass/wrapperClasses) against that same message.
```suggestion
String loadedExtensionPrefix =
"Loaded extension instance, type=" +
WrappedExt.class.getName() + ", name=impl1, instanceClass=";
String wrapperClassesInOrder =
", wrapperClasses=[" + Ext6Wrapper1.class.getName() + ",
" + Ext6Wrapper2.class.getName() + "]";
String wrapperClassesReversed =
", wrapperClasses=[" + Ext6Wrapper2.class.getName() + ",
" + Ext6Wrapper1.class.getName() + "]";
assertTrue(
logCollector.contains(loadedExtensionPrefix +
Ext6Wrapper1.class.getName() + wrapperClassesInOrder)
|| logCollector.contains(
loadedExtensionPrefix +
Ext6Wrapper1.class.getName() + wrapperClassesReversed)
|| logCollector.contains(
loadedExtensionPrefix +
Ext6Wrapper2.class.getName() + wrapperClassesInOrder)
|| logCollector.contains(
loadedExtensionPrefix +
Ext6Wrapper2.class.getName() + wrapperClassesReversed));
```
##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -1302,15 +1356,32 @@ private void loadClass(
cacheName(clazz, n);
saveInExtensionClass(extensionClasses, clazz, n,
overridden);
}
+ logLoadedExtensionClass("extension", clazz,
Arrays.asList(names), resourceURL, overridden);
Review Comment:
`logLoadedExtensionClass("extension", ...)` is invoked with
`Arrays.asList(names)` even when DEBUG is disabled, which still allocates a
list. Consider guarding the call with `logger.isDebugEnabled()` or passing the
raw array and converting inside the logging method after the debug check.
```suggestion
if (logger.isDebugEnabled()) {
logLoadedExtensionClass("extension", clazz,
Arrays.asList(names), resourceURL, overridden);
}
```
--
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]
