[
https://issues.apache.org/jira/browse/MRESOLVER-349?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17708784#comment-17708784
]
ASF GitHub Bot commented on MRESOLVER-349:
------------------------------------------
michael-o commented on code in PR #276:
URL: https://github.com/apache/maven-resolver/pull/276#discussion_r1158276327
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -130,47 +145,78 @@ private TimeUnit getTimeUnit(final
RepositorySystemSession session) {
return TimeUnit.valueOf(ConfigUtils.getString(session,
DEFAULT_TIME_UNIT.name(), TIME_UNIT_KEY));
}
+ private int getRetries(final RepositorySystemSession session) {
+ return ConfigUtils.getInteger(session, DEFAULT_RETRIES,
RETRIES_KEY);
+ }
+
+ private long getRetrySleep(final RepositorySystemSession session) {
+ return ConfigUtils.getLong(session, DEFAULT_RETRY_SLEEP,
RETRY_SLEEP_KEY);
+ }
+
@Override
public void acquire(Collection<? extends Artifact> artifacts,
Collection<? extends Metadata> metadatas) {
Collection<String> keys = lockNaming.nameLocks(session, artifacts,
metadatas);
if (keys.isEmpty()) {
return;
}
- LOGGER.trace("Need {} {} lock(s) for {}", keys.size(), shared ?
"read" : "write", keys);
- int acquiredLockCount = 0;
- for (String key : keys) {
- NamedLock namedLock = namedLockFactory.getLock(key);
- try {
- LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read"
: "write", key);
-
- boolean locked;
- if (shared) {
- locked = namedLock.lockShared(time, timeUnit);
- } else {
- locked = namedLock.lockExclusively(time, timeUnit);
- }
-
- if (!locked) {
- LOGGER.trace("Failed to acquire {} lock for '{}'",
shared ? "read" : "write", key);
-
- namedLock.close();
- throw new IllegalStateException("Could not acquire " +
(shared ? "read" : "write")
- + " lock for '" + namedLock.name() + "'");
+ final ArrayList<IllegalStateException> illegalStateExceptions =
new ArrayList<>();
+ for (int retry = 0; retry <= retries; retry++) {
+ LOGGER.trace(
+ "Attempt {}: Need {} {} lock(s) for {}", retry,
keys.size(), shared ? "read" : "write", keys);
+ int acquiredLockCount = 0;
+ for (String key : keys) {
+ NamedLock namedLock = namedLockFactory.getLock(key);
+ try {
+ if (retry > 0) {
+ Thread.sleep(retrySleep);
+ }
+ LOGGER.trace("Acquiring {} lock for '{}'", shared ?
"read" : "write", key);
+
+ boolean locked;
+ if (shared) {
+ locked = namedLock.lockShared(time, timeUnit);
+ } else {
+ locked = namedLock.lockExclusively(time, timeUnit);
+ }
+
+ if (!locked) {
+ String timeStr = time + " " + timeUnit;
+ LOGGER.trace(
+ "Failed to acquire {} lock for '{}' in {}",
+ shared ? "read" : "write",
+ key,
+ timeStr);
Review Comment:
This one I like!
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -130,47 +145,78 @@ private TimeUnit getTimeUnit(final
RepositorySystemSession session) {
return TimeUnit.valueOf(ConfigUtils.getString(session,
DEFAULT_TIME_UNIT.name(), TIME_UNIT_KEY));
}
+ private int getRetries(final RepositorySystemSession session) {
+ return ConfigUtils.getInteger(session, DEFAULT_RETRIES,
RETRIES_KEY);
+ }
+
+ private long getRetrySleep(final RepositorySystemSession session) {
+ return ConfigUtils.getLong(session, DEFAULT_RETRY_SLEEP,
RETRY_SLEEP_KEY);
+ }
+
@Override
public void acquire(Collection<? extends Artifact> artifacts,
Collection<? extends Metadata> metadatas) {
Collection<String> keys = lockNaming.nameLocks(session, artifacts,
metadatas);
if (keys.isEmpty()) {
return;
}
- LOGGER.trace("Need {} {} lock(s) for {}", keys.size(), shared ?
"read" : "write", keys);
- int acquiredLockCount = 0;
- for (String key : keys) {
- NamedLock namedLock = namedLockFactory.getLock(key);
- try {
- LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read"
: "write", key);
-
- boolean locked;
- if (shared) {
- locked = namedLock.lockShared(time, timeUnit);
- } else {
- locked = namedLock.lockExclusively(time, timeUnit);
- }
-
- if (!locked) {
- LOGGER.trace("Failed to acquire {} lock for '{}'",
shared ? "read" : "write", key);
-
- namedLock.close();
- throw new IllegalStateException("Could not acquire " +
(shared ? "read" : "write")
- + " lock for '" + namedLock.name() + "'");
+ final ArrayList<IllegalStateException> illegalStateExceptions =
new ArrayList<>();
+ for (int retry = 0; retry <= retries; retry++) {
+ LOGGER.trace(
+ "Attempt {}: Need {} {} lock(s) for {}", retry,
keys.size(), shared ? "read" : "write", keys);
+ int acquiredLockCount = 0;
+ for (String key : keys) {
+ NamedLock namedLock = namedLockFactory.getLock(key);
+ try {
+ if (retry > 0) {
+ Thread.sleep(retrySleep);
+ }
+ LOGGER.trace("Acquiring {} lock for '{}'", shared ?
"read" : "write", key);
+
+ boolean locked;
+ if (shared) {
+ locked = namedLock.lockShared(time, timeUnit);
+ } else {
+ locked = namedLock.lockExclusively(time, timeUnit);
+ }
+
+ if (!locked) {
+ String timeStr = time + " " + timeUnit;
+ LOGGER.trace(
+ "Failed to acquire {} lock for '{}' in {}",
+ shared ? "read" : "write",
+ key,
+ timeStr);
+
+ namedLock.close();
+ closeAll();
+ illegalStateExceptions.add(new
IllegalStateException(
+ "Attempt: " + retry + ": Could not acquire
" + (shared ? "read" : "write")
Review Comment:
`Attempt " + retry + ":` superfluous colon
##########
src/site/markdown/configuration.md:
##########
@@ -92,7 +92,9 @@ Option | Type | Description | Default Value | Supports Repo
ID Suffix
`aether.syncContext.named.factory` | String | Name of the named lock factory
implementing the `org.eclipse.aether.named.NamedLockFactory` interface. |
`"rwlock-local"` | no
`aether.syncContext.named.hashing.depth` | int | The directory depth to
"spread" hashes in git-like fashion, integer between 0 and 4 (inclusive). | 2 |
no
`aether.syncContext.named.nameMapper` | String | Name of name mapper
implementing the
`org.eclipse.aether.internal.impl.synccontext.named.NameMapper` interface. |
`"gav"` | no
-`aether.syncContext.named.time` | long | Amount of time a synchronization
context shall wait to obtain a lock. | 30 | no
+`aether.syncContext.named.retries` | int | Count of retries SyncContext
adapter should attempt, when obtaining locks. | `2` | no
+`aether.syncContext.named.retrySleep` | long | Count of milliseconds of thread
to sleep between attempts, when obtaining locks. | `200` | no
Review Comment:
* Maybe subordinate? `aether.syncContext.named.retry.wait`? We use "wait"
already, consistency.
* Amount of milliseconds a thread to wait between retries, when obtaining
locks.
You need to clarify wether you want to use the term "attempts" or "retries"
because they are not the same.
My understanding:
* Attempt: 2. Means first and second run
* Retries: 1 + 2 because the first is a try, subsequent are tries.
I think you mean attempts here.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -130,47 +145,78 @@ private TimeUnit getTimeUnit(final
RepositorySystemSession session) {
return TimeUnit.valueOf(ConfigUtils.getString(session,
DEFAULT_TIME_UNIT.name(), TIME_UNIT_KEY));
}
+ private int getRetries(final RepositorySystemSession session) {
+ return ConfigUtils.getInteger(session, DEFAULT_RETRIES,
RETRIES_KEY);
+ }
+
+ private long getRetrySleep(final RepositorySystemSession session) {
+ return ConfigUtils.getLong(session, DEFAULT_RETRY_SLEEP,
RETRY_SLEEP_KEY);
+ }
+
@Override
public void acquire(Collection<? extends Artifact> artifacts,
Collection<? extends Metadata> metadatas) {
Collection<String> keys = lockNaming.nameLocks(session, artifacts,
metadatas);
if (keys.isEmpty()) {
return;
}
- LOGGER.trace("Need {} {} lock(s) for {}", keys.size(), shared ?
"read" : "write", keys);
- int acquiredLockCount = 0;
- for (String key : keys) {
- NamedLock namedLock = namedLockFactory.getLock(key);
- try {
- LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read"
: "write", key);
-
- boolean locked;
- if (shared) {
- locked = namedLock.lockShared(time, timeUnit);
- } else {
- locked = namedLock.lockExclusively(time, timeUnit);
- }
-
- if (!locked) {
- LOGGER.trace("Failed to acquire {} lock for '{}'",
shared ? "read" : "write", key);
-
- namedLock.close();
- throw new IllegalStateException("Could not acquire " +
(shared ? "read" : "write")
- + " lock for '" + namedLock.name() + "'");
+ final ArrayList<IllegalStateException> illegalStateExceptions =
new ArrayList<>();
+ for (int retry = 0; retry <= retries; retry++) {
Review Comment:
This doesn't look right. It should be: `for (int retry = 1; retry <=
retries; retry++)` because what is attempt 0?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -130,47 +145,78 @@ private TimeUnit getTimeUnit(final
RepositorySystemSession session) {
return TimeUnit.valueOf(ConfigUtils.getString(session,
DEFAULT_TIME_UNIT.name(), TIME_UNIT_KEY));
}
+ private int getRetries(final RepositorySystemSession session) {
+ return ConfigUtils.getInteger(session, DEFAULT_RETRIES,
RETRIES_KEY);
+ }
+
+ private long getRetrySleep(final RepositorySystemSession session) {
+ return ConfigUtils.getLong(session, DEFAULT_RETRY_SLEEP,
RETRY_SLEEP_KEY);
+ }
+
@Override
public void acquire(Collection<? extends Artifact> artifacts,
Collection<? extends Metadata> metadatas) {
Collection<String> keys = lockNaming.nameLocks(session, artifacts,
metadatas);
if (keys.isEmpty()) {
return;
}
- LOGGER.trace("Need {} {} lock(s) for {}", keys.size(), shared ?
"read" : "write", keys);
- int acquiredLockCount = 0;
- for (String key : keys) {
- NamedLock namedLock = namedLockFactory.getLock(key);
- try {
- LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read"
: "write", key);
-
- boolean locked;
- if (shared) {
- locked = namedLock.lockShared(time, timeUnit);
- } else {
- locked = namedLock.lockExclusively(time, timeUnit);
- }
-
- if (!locked) {
- LOGGER.trace("Failed to acquire {} lock for '{}'",
shared ? "read" : "write", key);
-
- namedLock.close();
- throw new IllegalStateException("Could not acquire " +
(shared ? "read" : "write")
- + " lock for '" + namedLock.name() + "'");
+ final ArrayList<IllegalStateException> illegalStateExceptions =
new ArrayList<>();
+ for (int retry = 0; retry <= retries; retry++) {
+ LOGGER.trace(
+ "Attempt {}: Need {} {} lock(s) for {}", retry,
keys.size(), shared ? "read" : "write", keys);
+ int acquiredLockCount = 0;
+ for (String key : keys) {
+ NamedLock namedLock = namedLockFactory.getLock(key);
+ try {
+ if (retry > 0) {
Review Comment:
Then `> 1`
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -130,47 +145,78 @@ private TimeUnit getTimeUnit(final
RepositorySystemSession session) {
return TimeUnit.valueOf(ConfigUtils.getString(session,
DEFAULT_TIME_UNIT.name(), TIME_UNIT_KEY));
}
+ private int getRetries(final RepositorySystemSession session) {
+ return ConfigUtils.getInteger(session, DEFAULT_RETRIES,
RETRIES_KEY);
+ }
+
+ private long getRetrySleep(final RepositorySystemSession session) {
+ return ConfigUtils.getLong(session, DEFAULT_RETRY_SLEEP,
RETRY_SLEEP_KEY);
+ }
+
@Override
public void acquire(Collection<? extends Artifact> artifacts,
Collection<? extends Metadata> metadatas) {
Collection<String> keys = lockNaming.nameLocks(session, artifacts,
metadatas);
if (keys.isEmpty()) {
return;
}
- LOGGER.trace("Need {} {} lock(s) for {}", keys.size(), shared ?
"read" : "write", keys);
- int acquiredLockCount = 0;
- for (String key : keys) {
- NamedLock namedLock = namedLockFactory.getLock(key);
- try {
- LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read"
: "write", key);
-
- boolean locked;
- if (shared) {
- locked = namedLock.lockShared(time, timeUnit);
- } else {
- locked = namedLock.lockExclusively(time, timeUnit);
- }
-
- if (!locked) {
- LOGGER.trace("Failed to acquire {} lock for '{}'",
shared ? "read" : "write", key);
-
- namedLock.close();
- throw new IllegalStateException("Could not acquire " +
(shared ? "read" : "write")
- + " lock for '" + namedLock.name() + "'");
+ final ArrayList<IllegalStateException> illegalStateExceptions =
new ArrayList<>();
+ for (int retry = 0; retry <= retries; retry++) {
+ LOGGER.trace(
+ "Attempt {}: Need {} {} lock(s) for {}", retry,
keys.size(), shared ? "read" : "write", keys);
+ int acquiredLockCount = 0;
+ for (String key : keys) {
+ NamedLock namedLock = namedLockFactory.getLock(key);
+ try {
+ if (retry > 0) {
+ Thread.sleep(retrySleep);
+ }
+ LOGGER.trace("Acquiring {} lock for '{}'", shared ?
"read" : "write", key);
+
+ boolean locked;
+ if (shared) {
+ locked = namedLock.lockShared(time, timeUnit);
+ } else {
+ locked = namedLock.lockExclusively(time, timeUnit);
+ }
+
+ if (!locked) {
+ String timeStr = time + " " + timeUnit;
+ LOGGER.trace(
+ "Failed to acquire {} lock for '{}' in {}",
+ shared ? "read" : "write",
+ key,
+ timeStr);
+
+ namedLock.close();
+ closeAll();
+ illegalStateExceptions.add(new
IllegalStateException(
+ "Attempt: " + retry + ": Could not acquire
" + (shared ? "read" : "write")
+ + " lock for '" + namedLock.name()
+ "' in " + timeStr));
+ break;
+ } else {
+ locks.push(namedLock);
+ acquiredLockCount++;
+ }
+
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException(e);
}
-
- locks.push(namedLock);
- acquiredLockCount++;
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new RuntimeException(e);
}
+ LOGGER.trace("Attempt {}: Total locks acquired: {}", retry,
acquiredLockCount);
+ if (acquiredLockCount == keys.size()) {
+ break;
+ }
Review Comment:
Circuit breaker here, right?
> Adapter when locking should "give up and retry"
> -----------------------------------------------
>
> Key: MRESOLVER-349
> URL: https://issues.apache.org/jira/browse/MRESOLVER-349
> Project: Maven Resolver
> Issue Type: Task
> Components: Resolver
> Affects Versions: 1.9.7
> Reporter: Tamas Cservenak
> Assignee: Tamas Cservenak
> Priority: Major
> Fix For: 1.9.8
>
>
> Somewhat in relation to MRESOLVER-346, but sync context (the named lock
> adapter to be more specific) instead of holding as many it could locked so
> far (and cause lock timeouts), should simply give up all and retry.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
