You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "sebastien-doyon (via GitHub)" <gi...@apache.org> on 2023/08/31 14:09:12 UTC
[GitHub] [maven] sebastien-doyon opened a new pull request, #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
sebastien-doyon opened a new pull request, #1220:
URL: https://github.com/apache/maven/pull/1220
Use message formats with placeholders to avoid premature formatting and avoid the unnecessary garbage when then log level is disabled :
org.apache.maven.project.artifact.MavenMetadataSource
org.apache.maven.toolchain.DefaultToolchainsBuilder
org.apache.maven.DefaultMaven
org.apache.maven.classrealm.DefaultClassRealmManager
org.apache.maven.internal.aether.LoggingRepositoryListener
org.apache.maven.lifecycle.internal.DefaultLifecyclePluginAnalyzer
org.apache.maven.lifecycle.internal.LifecycleDependencyResolver
org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder
org.apache.maven.plugin.internal.DefaultMavenPluginManager
org.apache.maven.plugin.internal.DefaultPluginDependenciesResolver
org.apache.maven.plugin.prefix.internal.DefaultPluginPrefixResolver
org.apache.maven.plugin.version.internal.DefaultPluginVersionResolver
org.apache.maven.project.collector.MultiModuleCollectionStrategy
org.apache.maven.toolchain.DefaultToolchain
Guarding some logging statements with conditionals on isXXXXEnabled() to avoid unnecessary garbage when then log level is disabled. Useful when some formatting must be done outside the logger call :
org.apache.maven.classrealm.DefaultClassRealmManager
org.apache.maven.internal.impl.DefaultLog
org.apache.maven.lifecycle.DefaultLifecycles
org.apache.maven.plugin.internal.DefaultMavenPluginManager
org.apache.maven.plugin.internal.MojoLogWrapper
Remove some unneeded conditional guarding to avoid testing twice if the log level is enabled :
org.apache.maven.classrealm.DefaultClassRealmManager
org.apache.maven.lifecycle.DefaultLifecycles
org.apache.maven.plugin.prefix.internal.DefaultPluginPrefixResolver
org.apache.maven.plugin.version.internal.DefaultPluginVersionResolver
New test class :
org.apache.maven.classrealm.DefaultClassRealmManagerTest
Test class modified to reflect the new logger API usage :
org.apache.maven.toolchain.DefaultToolchainTest
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312969152
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,20 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = Collections.emptySet();
Review Comment:
I think in this case it is not a problem. The **artifactIds** reference a collection that stays local to the method, it is not escaping the class and is not an instance variable. Knowing that, the emptySet() is not modified in this state of the code. Do you agree on the safe usage?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1324732994
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,16 +149,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
-
- List<ClassRealmConstituent> constituents = new ArrayList<>();
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
if (artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
+ } else if (logger.isDebugEnabled()) {
+ logger.debug(" Excluded: {}", getId(artifact));
Review Comment:
Obviously better. Thanks for the suggestion. Will do the change later.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312963669
##########
maven-core/pom.xml:
##########
@@ -153,7 +153,7 @@ under the License.
</dependency>
<dependency>
<groupId>org.mockito</groupId>
- <artifactId>mockito-core</artifactId>
+ <artifactId>mockito-inline</artifactId>
Review Comment:
This was needed for the DefaultClassRealmManagerTest test class tube able to mock the static method LoggerFactory.getLogger(). According to your comment on this test class bellow, this should be reverted if you think the test is not wanted.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1316294272
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
LGTM
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1314461642
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
I would not make the computation of the artifactIds condition to the log level.
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +300,18 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm, List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = new LinkedHashSet<>(constituents.size());
- if (logger.isDebugEnabled()) {
- logger.debug("Populating class realm " + classRealm.getId());
- }
+ logger.debug("Populating class realm {}", classRealm.getId());
for (ClassRealmConstituent constituent : constituents) {
File file = constituent.getFile();
- String id = getId(constituent);
- includedIds.add(id);
-
if (logger.isDebugEnabled()) {
- logger.debug(" Included: " + id);
+ String id = getId(constituent);
Review Comment:
Same here. Move back the `includedIds` computation and simply remove the `isDebugEnabled()` call.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312882728
##########
maven-core/pom.xml:
##########
@@ -153,7 +153,7 @@ under the License.
</dependency>
<dependency>
<groupId>org.mockito</groupId>
- <artifactId>mockito-core</artifactId>
+ <artifactId>mockito-inline</artifactId>
Review Comment:
Is this change related?
##########
maven-core/src/test/java/org/apache/maven/classrealm/DefaultClassRealmManagerTest.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.maven.classrealm;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+
+import org.apache.maven.extension.internal.CoreExports;
+import org.apache.maven.model.Model;
+import org.codehaus.plexus.DefaultPlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.PlexusContainerException;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
+import org.eclipse.aether.artifact.Artifact;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.calls;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Sebastien Doyon
+ */
+class DefaultClassRealmManagerTest {
+
+ private DefaultClassRealmManager newDefaultClassRealmManager(PlexusContainer container) {
+ HashSet<String> exportedPackages = new HashSet<String>();
+ exportedPackages.add("group1:artifact1");
+
+ return new DefaultClassRealmManager(
+ container,
+ new ArrayList<ClassRealmManagerDelegate>(),
+ new CoreExports(new ClassRealm(null, "test", null), new HashSet<String>(), exportedPackages));
+ }
+
+ private List<Artifact> newTestArtifactList() {
+ List<Artifact> artifacts = new ArrayList<Artifact>();
+
+ Artifact artifact = mock(Artifact.class);
+ when(artifact.getFile()).thenReturn(new File(new File("local/repository"), "some/path"));
+ when(artifact.getGroupId()).thenReturn("group1");
+ when(artifact.getArtifactId()).thenReturn("artifact1");
+ when(artifact.getExtension()).thenReturn("ext");
+ when(artifact.getClassifier()).thenReturn("classifier1");
+ when(artifact.getVersion()).thenReturn("1");
+ artifacts.add(artifact);
+
+ Artifact artifact2 = mock(Artifact.class);
+ when(artifact2.getFile()).thenReturn(null);
+ when(artifact2.getGroupId()).thenReturn("group1");
+ when(artifact2.getArtifactId()).thenReturn("artifact2");
+ when(artifact2.getExtension()).thenReturn("ext");
+ when(artifact2.getClassifier()).thenReturn("classifier1");
+ when(artifact2.getVersion()).thenReturn("1");
+ artifacts.add(artifact2);
+
+ return artifacts;
+ }
+
+ private Model newTestModel() {
+ Model model = new Model();
+ model.setGroupId("modelGroup1");
+ model.setArtifactId("modelArtifact1");
+ model.setVersion("modelVersion1");
+
+ return model;
+ }
+
+ @Test
+ void testDebugEnabled() throws PlexusContainerException {
+ Logger logger = mock(Logger.class);
+ when(logger.isDebugEnabled()).thenReturn(true);
+
+ DefaultClassRealmManager classRealmManager;
+ ClassRealm classRealm;
+
+ InOrder verifier = inOrder(logger);
+
+ PlexusContainer container = new DefaultPlexusContainer();
+
+ try (MockedStatic<LoggerFactory> mockedLoggerFactory = Mockito.mockStatic(LoggerFactory.class)) {
Review Comment:
In general, I'm skeptical of testing log messages. I tend to think of them as side effects that can change pretty freely, and not part of a public API contract that should be tested.
##########
maven-core/src/main/java/org/apache/maven/plugin/internal/MojoLogWrapper.java:
##########
@@ -34,7 +34,9 @@ public MojoLogWrapper(Logger logger) {
}
public void debug(CharSequence content) {
- logger.debug(toString(content));
+ if (isDebugEnabled()) {
Review Comment:
I notice this duplicates a lot of content from DefaultLog above. I wonder if that's fixable somehow? Not necessarily in this PR.
##########
maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLog.java:
##########
@@ -33,12 +33,16 @@ public DefaultLog(Logger logger) {
}
public void debug(CharSequence content) {
- logger.debug(toString(content));
+ if (isDebugEnabled()) {
Review Comment:
I like this file's changes. They're nice and clean and in one place.
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,20 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = Collections.emptySet();
Review Comment:
slipping in an immutable set where a mutable one existed previously is risky. It's safer to just make it always a LinkedHashSet. I'm, not saying this case is wrong, but I have experienced a really nasty, hard to debug production outage by doing pretty much this.
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +305,22 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm, List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = Collections.emptySet();
Review Comment:
revert
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1314719901
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
Let me try to explain what was the point of this change. The artifactIds list is computed only for debugging purpose. By adding the condition on the log level it is avoiding the computation of building this list if logging is not going to happen. The same for includedIds in your other comment. They are both used only to compute the debug logs.
One other comment, while trying to reduce the complexity of this code and limit the number of isDebugEnabled() conditions, I came to the conclusion that something in this logic is fishy. If you look closely on the createRealm() method where the artifactIds list is populated and in the populateRealm() where the includedIds is populated. The only elements that would be in the artifactIds list and not in the includedIds list are artifacts with getFile() null value. If I am right the logic of this code can be reduced a lot, both limiting the isDebugEnabled() conditions and removing some list constructions in the populateRealm() method.
Please tell me what do you think about this idea. Did I miss something in the code? I can patch the PR with the simplified code if you agree.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1313425324
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +305,22 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm, List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = Collections.emptySet();
Review Comment:
Fine with me, thanks for clarifying. Reverting this line
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1313425780
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,20 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = Collections.emptySet();
Review Comment:
Reverting this line according to avoid problems
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1313207880
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +305,22 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm, List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = Collections.emptySet();
Review Comment:
You're saying that at this commit, mixing mutable and immutable lists is not a problem. That's likely true. However there's no guarantee this will still be true in a commit 5 years down the road. I've seen slipping an immutable list into something that wasn't expecting it cause very painful production problems. It's just safer to always use one or the other, and make sure the declared types make it clear which is being used. Any collection not explicitly declared as immutable should not be immutable. Yes, this is a design flaw in the Java Collections API going back to Java 1.1, but it's a design flaw we have to live with.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1315813792
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
I made a commit with the proposed changes (direct link [here](https://github.com/apache/maven/pull/1220/commits/9fd8b54a045a8b98fd7edad3bea010242a37340c)), please comment. This is reducing the number of isDebugEnabled() blocks and the collections used only for logging informations. The business logic is unchanged and the logged information is the same.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1324599510
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,16 +149,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
-
- List<ClassRealmConstituent> constituents = new ArrayList<>();
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
if (artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
+ } else if (logger.isDebugEnabled()) {
+ logger.debug(" Excluded: {}", getId(artifact));
Review Comment:
I think you are right, but to be sure all path that are not adding the artifact to the constituents list are logged as excluded, I can keep the condition here and add the same else block to the "if isProvidedArtifact" has you proposed. What do you think? Alternative fix could be a simple if after the"if isProvidedArtifact" if I add a continue; after the line 158. This is a matter of taste, what do you think?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312999364
##########
maven-core/src/test/java/org/apache/maven/classrealm/DefaultClassRealmManagerTest.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.maven.classrealm;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+
+import org.apache.maven.extension.internal.CoreExports;
+import org.apache.maven.model.Model;
+import org.codehaus.plexus.DefaultPlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.PlexusContainerException;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
+import org.eclipse.aether.artifact.Artifact;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.calls;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Sebastien Doyon
+ */
+class DefaultClassRealmManagerTest {
+
+ private DefaultClassRealmManager newDefaultClassRealmManager(PlexusContainer container) {
+ HashSet<String> exportedPackages = new HashSet<String>();
+ exportedPackages.add("group1:artifact1");
+
+ return new DefaultClassRealmManager(
+ container,
+ new ArrayList<ClassRealmManagerDelegate>(),
+ new CoreExports(new ClassRealm(null, "test", null), new HashSet<String>(), exportedPackages));
+ }
+
+ private List<Artifact> newTestArtifactList() {
+ List<Artifact> artifacts = new ArrayList<Artifact>();
+
+ Artifact artifact = mock(Artifact.class);
+ when(artifact.getFile()).thenReturn(new File(new File("local/repository"), "some/path"));
+ when(artifact.getGroupId()).thenReturn("group1");
+ when(artifact.getArtifactId()).thenReturn("artifact1");
+ when(artifact.getExtension()).thenReturn("ext");
+ when(artifact.getClassifier()).thenReturn("classifier1");
+ when(artifact.getVersion()).thenReturn("1");
+ artifacts.add(artifact);
+
+ Artifact artifact2 = mock(Artifact.class);
+ when(artifact2.getFile()).thenReturn(null);
+ when(artifact2.getGroupId()).thenReturn("group1");
+ when(artifact2.getArtifactId()).thenReturn("artifact2");
+ when(artifact2.getExtension()).thenReturn("ext");
+ when(artifact2.getClassifier()).thenReturn("classifier1");
+ when(artifact2.getVersion()).thenReturn("1");
+ artifacts.add(artifact2);
+
+ return artifacts;
+ }
+
+ private Model newTestModel() {
+ Model model = new Model();
+ model.setGroupId("modelGroup1");
+ model.setArtifactId("modelArtifact1");
+ model.setVersion("modelVersion1");
+
+ return model;
+ }
+
+ @Test
+ void testDebugEnabled() throws PlexusContainerException {
+ Logger logger = mock(Logger.class);
+ when(logger.isDebugEnabled()).thenReturn(true);
+
+ DefaultClassRealmManager classRealmManager;
+ ClassRealm classRealm;
+
+ InOrder verifier = inOrder(logger);
+
+ PlexusContainer container = new DefaultPlexusContainer();
+
+ try (MockedStatic<LoggerFactory> mockedLoggerFactory = Mockito.mockStatic(LoggerFactory.class)) {
Review Comment:
I agree with the principle. I did this test class because in the it was specified in the contribution guidelines : "Make sure you have added the necessary tests (JUnit/[Core IT tests](https://maven.apache.org/core-its/core-it-suite/)) for your changes.". And I found at least one other test example that was testing log messages, the DefaultToolchainTest class. I don't know if any external tools may be expecting maven to log in a specific format, like Maven Daemon? If you think the tests are a bad idea, I am willing to remove, just tell 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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1315813792
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
I made a commit with the proposed changes (direct link [here](maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java)), please comment. This is reducing the number of isDebugEnabled() blocks and the collections used only for logging informations. The business logic is unchanged and the logged information is the same.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1315813792
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = new LinkedHashSet<>(artifacts == null ? 0 : artifacts.size());
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- List<ClassRealmConstituent> constituents = new ArrayList<>();
-
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
+ if (logger.isDebugEnabled()) {
+ artifactIds.add(getId(artifact));
+ }
Review Comment:
I made a commit with the proposed changes (direct link [here](https://github.com/apache/maven/pull/1220/commits/3bf7436bb8862d824bc72865206bc6e447413cd3)), please comment. This is reducing the number of isDebugEnabled() blocks and the collections used only for logging informations. The business logic is unchanged and the logged information is the same.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet merged pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #1220:
URL: https://github.com/apache/maven/pull/1220
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312991288
##########
maven-core/src/main/java/org/apache/maven/plugin/internal/MojoLogWrapper.java:
##########
@@ -34,7 +34,9 @@ public MojoLogWrapper(Logger logger) {
}
public void debug(CharSequence content) {
- logger.debug(toString(content));
+ if (isDebugEnabled()) {
Review Comment:
Yes this is true, the code is mostly a duplicate of the class **DefaultLog**, but they are not implementing the same interface :
org.apache.maven.plugin.logging.Log
org.apache.maven.api.plugin.Log
Maybe it is possible to reorganise this code to remove the duplicates but this is probably like you said something for another PR
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312983567
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +305,22 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm, List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = Collections.emptySet();
Review Comment:
What is your concerns? The **includedIds** reference a collection that stays local to the methods **populateRealm** and **createRealm**, it is not escaping the class and is not an instance variable. The set is only modified if the logger debug log level is enabled (line 319 and 321), it is set with a LinkedHashSet on the same condition (lines 310 and 311). I see no point of creating a LinkedHashSet if the log level is not at least debug since this collection is only used for logging purpose. What do you think according to this?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1324637257
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,16 +149,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
-
- List<ClassRealmConstituent> constituents = new ArrayList<>();
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
if (artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
+ } else if (logger.isDebugEnabled()) {
+ logger.debug(" Excluded: {}", getId(artifact));
Review Comment:
What about merging the `if`s
```
if (!isProvidedArtifact(artifact) && artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
} else if (logger.isDebugEnabled()) {
logger.debug(" Excluded: {}", getId(artifact));
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1325372519
##########
maven-core/src/main/java/org/apache/maven/internal/aether/LoggingRepositoryListener.java:
##########
@@ -63,48 +63,47 @@ public void metadataResolved(RepositoryEvent event) {
public void metadataInvalid(RepositoryEvent event) {
Exception exception = event.getException();
- StringBuilder buffer = new StringBuilder(256);
- buffer.append("The metadata ");
+ Object metadata;
if (event.getMetadata().getFile() != null) {
- buffer.append(event.getMetadata().getFile());
+ metadata = event.getMetadata().getFile();
} else {
- buffer.append(event.getMetadata());
+ metadata = event.getMetadata();
}
+ String errorType = " is invalid";
if (exception instanceof FileNotFoundException) {
- buffer.append(" is inaccessible");
- } else {
- buffer.append(" is invalid");
+ errorType = " is inaccessible";
}
+ String msg = "";
if (exception != null) {
- buffer.append(": ");
- buffer.append(exception.getMessage());
+ msg = ": " + exception.getMessage();
}
if (logger.isDebugEnabled()) {
- logger.warn(buffer.toString(), exception);
+ logger.warn("The metadata {} {}{}", metadata, errorType, msg, exception);
} else {
- logger.warn(buffer.toString());
+ logger.warn("The metadata {} {}{}", metadata, errorType, msg);
}
}
@Override
public void artifactDescriptorInvalid(RepositoryEvent event) {
- StringBuilder buffer = new StringBuilder(256);
- buffer.append("The POM for ");
- buffer.append(event.getArtifact());
- buffer.append(" is invalid, transitive dependencies (if any) will not be available");
-
if (logger.isDebugEnabled()) {
- logger.warn(buffer + ": " + event.getException().getMessage());
+ logger.warn(
+ "The POM for {} is invalid, transitive dependencies (if any) will not be available: {}",
+ event.getArtifact(),
+ event.getException().getMessage());
} else {
- logger.warn(buffer + ", enable verbose output (-X) for more details");
+ logger.warn(
+ "The POM for {} is invalid, transitive dependencies (if any) will not be available, enable "
+ + "verbose output (-X) for more details",
+ event.getArtifact());
Review Comment:
The code above is wrong and does not make much sense. To be consistent with other parts of this class, the whole method should look like:
```
if (logger.isDebugEnabled()) {
logger.warn(
"The POM for {} is invalid, transitive dependencies (if any) will not be available: {}",
event.getArtifact(),
event.getException().getMessage(),
event.getException());
} else {
logger.warn(
"The POM for {} is invalid, transitive dependencies (if any) will not be available: {}",
event.getArtifact(),
event.getException().getMessage());
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1220: [MNG-7866] Improvements to the logging API usage (technical debt)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1321137864
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,16 +149,15 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
-
- List<ClassRealmConstituent> constituents = new ArrayList<>();
+ List<ClassRealmConstituent> constituents = new ArrayList<>(artifacts == null ? 0 : artifacts.size());
- if (artifacts != null) {
+ if (artifacts != null && !artifacts.isEmpty()) {
for (Artifact artifact : artifacts) {
if (!isProvidedArtifact(artifact)) {
- artifactIds.add(getId(artifact));
if (artifact.getFile() != null) {
constituents.add(new ArtifactClassRealmConstituent(artifact));
+ } else if (logger.isDebugEnabled()) {
+ logger.debug(" Excluded: {}", getId(artifact));
Review Comment:
Re-reading this PR before merging, I think this is slightly wrong.
I think artifacts should be considered excluded and logged if `isProvidedArtifact` returns `true`, which I think is the main reason for the exclusion, as they kinda are supposed to always point to a file.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org