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 17:07:46 UTC
[GitHub] [maven-resolver] michael-o commented on a change in pull request #101: [MRESOLVER-153] Make TrackingFileManager use NamedLocks
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