You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "michael-o (via GitHub)" <gi...@apache.org> on 2023/04/05 09:47:44 UTC
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #276: [MRESOLVER-349] Adapter should give up and retry
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?
--
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