You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/04/27 13:00:40 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
cstamas opened a new pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101
Now that TrackingFileManager is shared singleton, let's make it use NamedLockFactory.
Commits:
Step1: drop all SyncContextFactory implementations and indirections, redo the same functionality using NamedLocks.
Step2: Pull out NamedLockFactory selection and expose selected one.
Step3: Tie up selected NamedLockFactory to TrackingFileManager.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#discussion_r621489660
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -228,7 +205,7 @@ public Properties update( File file, Map<String, String> updates )
}
else
{
- throw new IllegalStateException( "Could not acquire shared lock for: " + file );
+ throw new IllegalStateException( "Could not acquire exclusive lock for: " + file );
Review comment:
This would be 'write' like the rest of the exceptions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#discussion_r621312209
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
Review comment:
`getCanonicalPath()` comes at high cost some FS/OS. I wouldn't do that. the previous `TrackingFileManager` suffered from the same penalty. I don't really want to reintroduce it again.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
Review comment:
General question: Why not keep the prefix and the path, w/o checksum? The only benefit I see is same length lock names, otherwise it comes with computational costs.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
}
catch ( IOException e )
Review comment:
Who throws `IOException` here?
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactorySelector.java
##########
@@ -19,106 +19,115 @@
* under the License.
*/
-import org.eclipse.aether.RepositorySystemSession;
-import org.eclipse.aether.SyncContext;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
import org.eclipse.aether.internal.impl.synccontext.named.DiscriminatingNameMapper;
import org.eclipse.aether.internal.impl.synccontext.named.GAVNameMapper;
import org.eclipse.aether.internal.impl.synccontext.named.NameMapper;
-import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapter;
import org.eclipse.aether.internal.impl.synccontext.named.StaticNameMapper;
import org.eclipse.aether.named.NamedLockFactory;
import org.eclipse.aether.named.providers.LocalReadWriteLockNamedLockFactory;
import org.eclipse.aether.named.providers.LocalSemaphoreNamedLockFactory;
-
-import javax.annotation.PreDestroy;
-import javax.inject.Inject;
-import javax.inject.Named;
-import javax.inject.Singleton;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.concurrent.TimeUnit;
+import org.eclipse.aether.named.providers.NoopNamedLockFactory;
/**
- * Named {@link SyncContextFactoryDelegate} implementation that selects underlying {@link NamedLockFactory}
- * implementation at creation.
+ * Selector for {@link NamedLockFactory} and {@link NameMapper} that selects and exposes selected ones. Essentiall
Review comment:
Essential
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
}
catch ( IOException e )
{
- LOGGER.warn( "Failed to read tracking file {}", file, e );
+ throw new UncheckedIOException( e );
}
- finally
- {
- close( stream, file );
- }
-
- return null;
}
@Override
- public Properties update( File file, Map<String, String> updates )
+ public Properties read( File file )
{
- Properties props = new Properties();
-
- File directory = file.getParentFile();
- if ( !directory.mkdirs() && !directory.exists() )
- {
- LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
- return props;
- }
-
- RandomAccessFile raf = null;
- try
+ try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
Review comment:
Use selector here?
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
}
catch ( IOException e )
{
- LOGGER.warn( "Failed to read tracking file {}", file, e );
+ throw new UncheckedIOException( e );
}
- finally
- {
- close( stream, file );
- }
-
- return null;
}
@Override
- public Properties update( File file, Map<String, String> updates )
+ public Properties read( File file )
{
- Properties props = new Properties();
-
- File directory = file.getParentFile();
- if ( !directory.mkdirs() && !directory.exists() )
- {
- LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
- return props;
- }
-
- RandomAccessFile raf = null;
- try
+ try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
Review comment:
This suffers now from one problem. Since we don't use name mappers you will have the SAME lock name on two different CI nodes with Redisson using the same directory structure. They will block each other for no reason. Makes sense?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#issuecomment-829945807
Superseded by these two (sorry, as one depends on another, I could put it into my fork only, as I started all this work on a fork):
1st: https://github.com/apache/maven-resolver/pull/102
2nd: https://github.com/cstamas/maven-resolver/pull/3
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#discussion_r621490505
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
}
catch ( IOException e )
{
- LOGGER.warn( "Failed to read tracking file {}", file, e );
+ throw new UncheckedIOException( e );
}
- finally
- {
- close( stream, file );
- }
-
- return null;
}
@Override
- public Properties update( File file, Map<String, String> updates )
+ public Properties read( File file )
{
- Properties props = new Properties();
-
- File directory = file.getParentFile();
- if ( !directory.mkdirs() && !directory.exists() )
- {
- LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
- return props;
- }
-
- RandomAccessFile raf = null;
- try
+ try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
Review comment:
My point is that I expect consistent approach to name mapping wherever the lock factory is used. I need to think about this more tomorrow.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#issuecomment-829542955
@cstamas note to myself: split this in two: drop SyncContextFactory imples (other than named), and implement locking in TFM
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas closed pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
cstamas closed pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#discussion_r621435220
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -41,100 +52,189 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
+ private static final String LOCK_PREFIX = "tracking:";
+
+ private NamedLockFactory namedLockFactory;
+
+ public DefaultTrackingFileManager()
+ {
+ // ctor for ServiceLocator
+ }
+
+ @Inject
+ public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
+ {
+ this.namedLockFactory = selector.getSelectedNamedLockFactory();
+ }
+
@Override
- public Properties read( File file )
+ public void initService( final ServiceLocator locator )
+ {
+ NamedLockFactorySelector select = Objects.requireNonNull(
+ locator.getService( NamedLockFactorySelector.class ) );
+ this.namedLockFactory = select.getSelectedNamedLockFactory();
+ }
+
+ private String getFileKey( final File file )
{
- FileInputStream stream = null;
try
{
- if ( !file.exists() )
+ Map<String, Object> checksums = ChecksumUtils.calc(
+ file.getCanonicalPath().getBytes( StandardCharsets.UTF_8 ),
+ Collections.singletonList( "SHA-1" ) );
+ Object checksum = checksums.get( "SHA-1" );
+ if ( checksum instanceof RuntimeException )
{
- return null;
+ throw (RuntimeException) checksum;
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
+ else if ( checksum instanceof Exception )
+ {
+ throw new RuntimeException( ( Exception ) checksum );
+ }
+ return LOCK_PREFIX + checksum;
}
catch ( IOException e )
{
- LOGGER.warn( "Failed to read tracking file {}", file, e );
+ throw new UncheckedIOException( e );
}
- finally
- {
- close( stream, file );
- }
-
- return null;
}
@Override
- public Properties update( File file, Map<String, String> updates )
+ public Properties read( File file )
{
- Properties props = new Properties();
-
- File directory = file.getParentFile();
- if ( !directory.mkdirs() && !directory.exists() )
- {
- LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
- return props;
- }
-
- RandomAccessFile raf = null;
- try
+ try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
Review comment:
on two **different** nodes using two **different** storage mounts (disks), why'd you use redisson to sync those two (unrelated) nodes at all?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #101:
URL: https://github.com/apache/maven-resolver/pull/101#discussion_r621489660
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
##########
@@ -228,7 +205,7 @@ public Properties update( File file, Map<String, String> updates )
}
else
{
- throw new IllegalStateException( "Could not acquire shared lock for: " + file );
+ throw new IllegalStateException( "Could not acquire exclusive lock for: " + file );
Review comment:
This would be 'write' like the reset of the exceptions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org