[GitHub] [maven-resolver] gnodet commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure

[via GitHub]

gnodet commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1227686061


##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockSupport.java:
##
@@ -32,18 +39,74 @@ public abstract class NamedLockSupport implements NamedLock 
{
 
 private final NamedLockFactorySupport factory;
 
+private final ConcurrentHashMap> diagnosticState; // 
non-null only if diag enabled

Review Comment:
   Should we use a `ThreadLocal` ?
   That would simplify the code a bit imho (untested):
   ```
   private final ThreadLocal diagnosticState = 
factory.isDiagnosticEnabled() ? ThreadLocal.withInitial(ArrayDeque::new) : null;
   ```
   and then
   ```
   if (diagnosticState != null) {
   diagnosticState.get().push("shared");
   }
   ```



-- 
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-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-resolver] gnodet commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure

[via GitHub]

gnodet commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1226865171


##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockFactorySupport.java:
##
@@ -34,6 +35,8 @@
 public abstract class NamedLockFactorySupport implements NamedLockFactory {
 protected final Logger logger = LoggerFactory.getLogger(getClass());
 
+final boolean diagnostic = logger.isDebugEnabled();

Review Comment:
   That's a bad idea.  Loggers level can be updated at runtime, and the call to 
`isDebugEnabled()` should be quite enough to not warrant caching the value.



##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockFactorySupport.java:
##
@@ -34,6 +35,8 @@
 public abstract class NamedLockFactorySupport implements NamedLockFactory {
 protected final Logger logger = LoggerFactory.getLogger(getClass());
 
+final boolean diagnostic = logger.isDebugEnabled();

Review Comment:
   That's a bad idea.  Loggers level can be updated at runtime, and the call to 
`isDebugEnabled()` should be fast enough to not warrant caching the value.



-- 
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-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-resolver] gnodet commented on a diff in pull request #299: [MRESOLVER-370] Lock factory diagnostic on failure

[via GitHub]

gnodet commented on code in PR #299:
URL: https://github.com/apache/maven-resolver/pull/299#discussion_r1226656453


##
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockSupport.java:
##
@@ -32,18 +38,76 @@ public abstract class NamedLockSupport implements NamedLock 
{
 
 private final NamedLockFactorySupport factory;
 
+private final ConcurrentHashMap> state;
+
 public NamedLockSupport(final String name, final NamedLockFactorySupport 
factory) {
 this.name = name;
 this.factory = factory;
+this.state = factory.diagnostic ? new ConcurrentHashMap<>() : null;
 }
 
 @Override
 public String name() {
 return name;
 }
 
+@Override
+public boolean lockShared(long time, TimeUnit unit) throws 
InterruptedException {
+if (state != null) {
+state.computeIfAbsent(Thread.currentThread(), k -> new 
ArrayDeque<>())
+.push("S");

Review Comment:
   if we don't expect more than a few locks per thread at the same time, I 
would use `shared` and `exclusive`, it will not require the reader to go back 
to the source to understand what S/X means.



-- 
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-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org