Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#issuecomment-1909933056 @garydgregory, Do you have some suggestions on the naming of the new `Configuration#addExtensionIfAbsent` and `Configuration#getExtension` methods? I was thinking about renaming the first `registerExtension`, while the second seems to be similar to OSGi's [`Bundle#adapt`](https://docs.osgi.org/javadoc/osgi.core/8.0.0/org/osgi/framework/Bundle.html#adapt-java.lang.Class-) method. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#issuecomment-1908834695 Part of #2163 -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz merged PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230 -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1465405877 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; Review Comment: I changed it to a list. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1465008485 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; +} + +@Override +public T getExtension(Class extensionType) { +T result = null; +for (final ConfigurationExtension extension : extensions) { +if (extensionType.isInstance(extension)) { +if (result == null) { +result = (T) extension; +} else { +LOGGER.warn( +"Multiple configuration elements found for type {}. Only the first will be used.", +extensionType.getName()); +} +} +} +return result; +} Review Comment: IMHO this unexpected condition should be dealt in a uniform way. E.g. if a user adds multiple `` elements all but one should be ignored and a warning should be issued. This is better than the logic for multiple `` elements, where all but one are ignored **without** a warning. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464991374 ## log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorConfiguration.java: ## @@ -0,0 +1,132 @@ +/* + * 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.logging.log4j.core.async; + +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.AbstractLifeCycle; +import org.apache.logging.log4j.core.config.ConfigurationExtension; +import org.apache.logging.log4j.plugins.Configurable; +import org.apache.logging.log4j.plugins.Plugin; +import org.apache.logging.log4j.plugins.PluginAliases; +import org.apache.logging.log4j.plugins.PluginBuilderAttribute; +import org.apache.logging.log4j.plugins.PluginFactory; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.LoaderUtil; + +@Configurable(printObject = true) +@Plugin("Disruptor") +@PluginAliases("AsyncWaitStrategyFactory") +public final class DisruptorConfiguration extends AbstractLifeCycle implements ConfigurationExtension { + +private static final Logger LOGGER = StatusLogger.getLogger(); + +private final AsyncWaitStrategyFactory waitStrategyFactory; +private AsyncLoggerConfigDisruptor loggerConfigDisruptor; + +private DisruptorConfiguration(final AsyncWaitStrategyFactory waitStrategyFactory) { +this.waitStrategyFactory = waitStrategyFactory; +} + +public AsyncWaitStrategyFactory getWaitStrategyFactory() { +return waitStrategyFactory; +} + +public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() { +if (loggerConfigDisruptor == null) { +loggerConfigDisruptor = new AsyncLoggerConfigDisruptor(waitStrategyFactory); +} Review Comment: This was moved from: https://github.com/apache/logging-log4j2/blob/03f3aebe5a6775de5a0b087465ebd981fc869056/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java#L249-L256 The disruptor is created only if it is needed, so I would go for `Lazy` here. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464974551 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -732,12 +705,12 @@ protected void doConfigure() { final List copy = new ArrayList<>(customLevels); copy.add(child.getObject(CustomLevelConfig.class)); customLevels = copy; -} else if (child.isInstanceOf(AsyncWaitStrategyFactoryConfig.class)) { -AsyncWaitStrategyFactoryConfig awsfc = child.getObject(AsyncWaitStrategyFactoryConfig.class); -if (awsfc == null) { -LOGGER.error("Unable to load AsyncWaitStrategyFactoryConfig"); +} else if (child.isInstanceOf(ConfigurationExtension.class)) { +final ConfigurationExtension extension = child.getObject(ConfigurationExtension.class); +if (extension == null) { Review Comment: I had the same doubt, but I copypasted the `CustomLevels` code. ;-) Besides `getObject()` can not be `null` due to the null-check above these lines. ## log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorConfiguration.java: ## @@ -0,0 +1,132 @@ +/* + * 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.logging.log4j.core.async; + +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.AbstractLifeCycle; +import org.apache.logging.log4j.core.config.ConfigurationExtension; +import org.apache.logging.log4j.plugins.Configurable; +import org.apache.logging.log4j.plugins.Plugin; +import org.apache.logging.log4j.plugins.PluginAliases; +import org.apache.logging.log4j.plugins.PluginBuilderAttribute; +import org.apache.logging.log4j.plugins.PluginFactory; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.LoaderUtil; + +@Configurable(printObject = true) +@Plugin("Disruptor") +@PluginAliases("AsyncWaitStrategyFactory") +public final class DisruptorConfiguration extends AbstractLifeCycle implements ConfigurationExtension { + +private static final Logger LOGGER = StatusLogger.getLogger(); + +private final AsyncWaitStrategyFactory waitStrategyFactory; +private AsyncLoggerConfigDisruptor loggerConfigDisruptor; + +private DisruptorConfiguration(final AsyncWaitStrategyFactory waitStrategyFactory) { +this.waitStrategyFactory = waitStrategyFactory; +} + +public AsyncWaitStrategyFactory getWaitStrategyFactory() { +return waitStrategyFactory; +} + +public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() { +if (loggerConfigDisruptor == null) { +loggerConfigDisruptor = new AsyncLoggerConfigDisruptor(waitStrategyFactory); +} +return loggerConfigDisruptor; +} + +@Override +public void start() { +if (loggerConfigDisruptor != null) { +LOGGER.info("Starting AsyncLoggerConfigDisruptor."); +loggerConfigDisruptor.start(); +} +super.start(); +} + +@Override +public boolean stop(long timeout, TimeUnit timeUnit) { +if (loggerConfigDisruptor != null) { +LOGGER.info("Stopping AsyncLoggerConfigDisruptor."); +loggerConfigDisruptor.stop(timeout, timeUnit); +} +return super.stop(timeout, timeUnit); +} + +@PluginFactory +public static Builder newBuilder() { +return new Builder(); +} + +public static final class Builder implements org.apache.logging.log4j.plugins.util.Builder { + +@PluginBuilderAttribute("class") +private String factoryClassName; + +@PluginBuilderAttribute +private String waitFactory; + +public Builder setFactoryClassName(final String factoryClassName) { +this.factoryClassName = factoryClassName; +return this; +} + +public Builder setWaitFactory(final String waitFactory) { +this.waitFactory = waitFactory; +return this; +} + +
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
vy commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464849027 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; +} + +@Override +public T getExtension(Class extensionType) { +T result = null; +for (final ConfigurationExtension extension : extensions) { +if (extensionType.isInstance(extension)) { +if (result == null) { +result = (T) extension; +} else { +LOGGER.warn( +"Multiple configuration elements found for type {}. Only the first will be used.", +extensionType.getName()); +} +} +} +return result; +} Review Comment: > My idea behind this is to prevent plugin implementors from abusing this feature by adding a lot of new elements to the `` tag. I understand the concern. Though my point still stands: suppressing unexpected conditions is... unexpected, and, hence, not right. We can also address your concern by simply failing, which would be of my preference. For the record, let's also agree that _"plugin implementors"_ is a handful of people around the globe. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
vy commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464843982 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; Review Comment: @ppkarwasz, I would prefer a `List`. But leaving the decision to you. You can resolve this conversation as you wish. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464839041 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; +} + +@Override +public T getExtension(Class extensionType) { +T result = null; +for (final ConfigurationExtension extension : extensions) { +if (extensionType.isInstance(extension)) { +if (result == null) { +result = (T) extension; +} else { +LOGGER.warn( +"Multiple configuration elements found for type {}. Only the first will be used.", +extensionType.getName()); +} +} +} +return result; +} Review Comment: My idea behind this is to prevent plugin implementors from abusing this feature by adding a lot of new elements to the `` tag. Let's say an extension wants to add multiple `` elements to a configuration. Then I would strongly prefer to put them in a **single** `` wrapper element, instead of polluting the main `` element. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464828156 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationExtension.java: ## @@ -0,0 +1,22 @@ +/* + * 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.logging.log4j.core.config; + +/** + * A {@link ConfigurationExtension} is used to add new child elements to a {@link Configuration}. Review Comment: Here I was thinking on how it will appears in a documentation page: ## ConfigurationExtension A `ConfigurationExtension` is any element that can be added as child of a `` element. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
ppkarwasz commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464826082 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; Review Comment: No specific reason. -- 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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add general `ConfigurationExtension` mechanism (logging-log4j2)
vy commented on code in PR #2230: URL: https://github.com/apache/logging-log4j2/pull/2230#discussion_r1464763541 ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -1178,4 +1152,28 @@ public NanoClock getNanoClock() { public void setNanoClock(final NanoClock nanoClock) { instanceFactory.registerBinding(NanoClock.KEY, () -> nanoClock); } + +@Override +public void addExtension(ConfigurationExtension extension) { +Objects.requireNonNull(extension); +extensions = Arrays.copyOf(extensions, extensions.length + 1); +extensions[extensions.length - 1] = extension; Review Comment: Why is `extensions` not of type `List`? ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java: ## @@ -256,4 +236,21 @@ default LogEventFactory getLogEventFactory() { default RecyclerFactory getRecyclerFactory() { return getComponent(Key.forClass(RecyclerFactory.class)); } + +/** + * Registers a new configuration extension. + * + * @param extension a configuration extension. + * @since 3.0 + */ +void addExtension(ConfigurationExtension extension); + +/** + * Returns an extension of the given type. + * + * @param extensionType a type of extension, + * @return an extension the matches the given type. + * @since 3.0 + */ + T getExtension(Class extensionType); Review Comment: [In accordance with my comment above regarding suppressing unexpected multiple matches,] I would rather replace this with a simple `Collection getExtensions()`. ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationExtension.java: ## @@ -0,0 +1,22 @@ +/* + * 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.logging.log4j.core.config; + +/** + * A {@link ConfigurationExtension} is used to add new child elements to a {@link Configuration}. Review Comment: Nit: Shall we avoid recursive definitions, please? ```suggestion * The contract to add new child elements to a {@link Configuration}. ``` ## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ## @@ -732,12 +705,12 @@ protected void doConfigure() { final List copy = new ArrayList<>(customLevels); copy.add(child.getObject(CustomLevelConfig.class)); customLevels = copy; -} else if (child.isInstanceOf(AsyncWaitStrategyFactoryConfig.class)) { -AsyncWaitStrategyFactoryConfig awsfc = child.getObject(AsyncWaitStrategyFactoryConfig.class); -if (awsfc == null) { -LOGGER.error("Unable to load AsyncWaitStrategyFactoryConfig"); +} else if (child.isInstanceOf(ConfigurationExtension.class)) { +final ConfigurationExtension extension = child.getObject(ConfigurationExtension.class); +if (extension == null) { Review Comment: Checking `Node.java`... Can this ever be null? That is, can `child.isInstanceOf(T.class) && child.getObject(T.class) == null` ever return `true`? ## log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorConfiguration.java: ## @@ -0,0 +1,132 @@ +/* + * 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