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 2022/11/11 13:45:33 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #218: Expand use of atomic file ops
cstamas opened a new pull request, #218:
URL: https://github.com/apache/maven-resolver/pull/218
Drop old and legacy code doing smart(er) things, use common shared primitives instead.
--
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
[GitHub] [maven-resolver] slawekjaranowski commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020813023
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -241,8 +149,10 @@ private long copy( OutputStream os, InputStream is, ProgressListener listener )
return total;
}
+ @Deprecated
Review Comment:
We should add description - why is deprecated, what replacement ... and so.
later nobody will remember a reason
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020755905
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
Review Comment:
I would actually go other way around: make almost everything use FileProcessor (and extend it with some of things coming from FileUtils), as FileProcessor is a component, and hence, is replaceable. So, one could if needed, wrap the original component and "listen" for file changes happening. This is impossible with static helper....
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020813439
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -241,8 +149,10 @@ private long copy( OutputStream os, InputStream is, ProgressListener listener )
return total;
}
+ @Deprecated
Review Comment:
I undid deprecation
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1021271406
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -41,66 +42,55 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
@Override
public Properties read( File file )
{
- FileInputStream stream = null;
- try
+ Path filePath = file.toPath();
+ if ( Files.isRegularFile( filePath ) )
{
- if ( !file.exists() )
+ try ( InputStream stream = Files.newInputStream( filePath ) )
{
- return null;
+ Properties props = new Properties();
+ props.load( stream );
+ return props;
+ }
+ catch ( IOException e )
+ {
+ LOGGER.warn( "Failed to read tracking file '{}'", file, e );
+ throw new UncheckedIOException( e );
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
- }
- catch ( IOException e )
- {
- LOGGER.warn( "Failed to read tracking file {}", file, e );
- }
- finally
- {
- close( stream, file );
}
-
return null;
}
@Override
public Properties update( File file, Map<String, String> updates )
Review Comment:
Atomic files are not to circumvent locking at all, they are to guarantee that all (like downloads) are "all or nothing" even in case of errors, resolver will not leave partially written file under any circumstances (as partially written tmp file is deleted if fails).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -41,66 +42,55 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
@Override
public Properties read( File file )
{
- FileInputStream stream = null;
- try
+ Path filePath = file.toPath();
+ if ( Files.isRegularFile( filePath ) )
{
- if ( !file.exists() )
+ try ( InputStream stream = Files.newInputStream( filePath ) )
{
- return null;
+ Properties props = new Properties();
+ props.load( stream );
+ return props;
+ }
+ catch ( IOException e )
+ {
+ LOGGER.warn( "Failed to read tracking file '{}'", file, e );
+ throw new UncheckedIOException( e );
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
- }
- catch ( IOException e )
- {
- LOGGER.warn( "Failed to read tracking file {}", file, e );
- }
- finally
- {
- close( stream, file );
}
-
return null;
}
@Override
public Properties update( File file, Map<String, String> updates )
Review Comment:
Atomic files are not to circumvent locking at all, they are to guarantee that all fileops (like downloads) are "all or nothing" even in case of errors, resolver will not leave partially written file under any circumstances (as partially written tmp file is deleted if fails).
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020801363
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
Review Comment:
Not now but in near future https://issues.apache.org/jira/browse/MRESOLVER-295
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020913045
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -241,8 +149,10 @@ private long copy( OutputStream os, InputStream is, ProgressListener listener )
return total;
}
+ @Deprecated
Review Comment:
fixed
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1021193461
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
}
public void write( File target, InputStream source )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- copy( out, source, null );
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.copy( source, p ) );
Review Comment:
Do we actually need oneline wrappers for `Files` at all?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -41,66 +42,55 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
@Override
public Properties read( File file )
{
- FileInputStream stream = null;
- try
+ Path filePath = file.toPath();
+ if ( Files.isRegularFile( filePath ) )
{
- if ( !file.exists() )
+ try ( InputStream stream = Files.newInputStream( filePath ) )
{
- return null;
+ Properties props = new Properties();
+ props.load( stream );
+ return props;
+ }
+ catch ( IOException e )
+ {
+ LOGGER.warn( "Failed to read tracking file '{}'", file, e );
+ throw new UncheckedIOException( e );
}
-
- stream = new FileInputStream( file );
-
- Properties props = new Properties();
- props.load( stream );
-
- return props;
- }
- catch ( IOException e )
- {
- LOGGER.warn( "Failed to read tracking file {}", file, e );
- }
- finally
- {
- close( stream, file );
}
-
return null;
}
@Override
public Properties update( File file, Map<String, String> updates )
Review Comment:
Since this is run under `SyncContext` it is guaranteed to be locked, so atomic moves aren't required?!
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -41,66 +42,55 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
@Override
public Properties read( File file )
{
- FileInputStream stream = null;
- try
+ Path filePath = file.toPath();
+ if ( Files.isRegularFile( filePath ) )
{
- if ( !file.exists() )
+ try ( InputStream stream = Files.newInputStream( filePath ) )
{
- return null;
+ Properties props = new Properties();
+ props.load( stream );
+ return props;
+ }
+ catch ( IOException e )
+ {
+ LOGGER.warn( "Failed to read tracking file '{}'", file, e );
Review Comment:
This pattern should be then consistent throughout, means file param should be always in single quotes.
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020813468
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -241,8 +149,10 @@ private long copy( OutputStream os, InputStream is, ProgressListener listener )
return total;
}
+ @Deprecated
Review Comment:
Or missed this one?
--
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
[GitHub] [maven-resolver] cstamas merged pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas merged PR #218:
URL: https://github.com/apache/maven-resolver/pull/218
--
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
[GitHub] [maven-resolver] slawekjaranowski commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020740896
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -455,9 +456,7 @@ public boolean fetchChecksum( URI remote, File local )
protected void runTask()
throws Exception
{
- fileProcessor.mkdirs( file.getParentFile() );
Review Comment:
Looks like not used in production code `org.eclipse.aether.spi.io.FileProcessor#mkdirs`
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -489,7 +488,7 @@ protected void runTask()
}
}
}
- fileProcessor.move( tmp, file );
Review Comment:
Looks like not used anymore - `org.eclipse.aether.spi.io.FileProcessor#move`
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
Review Comment:
Why not use directly `FileUtils..`
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1021273680
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
}
public void write( File target, InputStream source )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- copy( out, source, null );
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.copy( source, p ) );
Review Comment:
File|Utils leverages TempFile, there is no equivalent for it in Files...
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1021304279
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultFileProcessor.java:
##########
@@ -75,144 +78,49 @@ public boolean mkdirs( File directory )
}
catch ( IOException e )
{
- return false;
+ throw new UncheckedIOException( e );
}
File parentDir = canonDir.getParentFile();
return ( parentDir != null && ( mkdirs( parentDir ) || parentDir.exists() ) && canonDir.mkdir() );
}
public void write( File target, String data )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- if ( data != null )
- {
- out.write( data.getBytes( StandardCharsets.UTF_8 ) );
- }
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.write( p, data.getBytes( StandardCharsets.UTF_8 ) ) );
}
public void write( File target, InputStream source )
- throws IOException
+ throws IOException
{
- mkdirs( target.getAbsoluteFile().getParentFile() );
-
- OutputStream out = null;
- try
- {
- out = new FileOutputStream( target );
-
- copy( out, source, null );
-
- out.close();
- out = null;
- }
- finally
- {
- try
- {
- if ( out != null )
- {
- out.close();
- }
- }
- catch ( final IOException e )
- {
- // Suppressed due to an exception already thrown in the try block.
- }
- }
+ FileUtils.writeFile( target.toPath(), p -> Files.copy( source, p ) );
Review Comment:
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
[GitHub] [maven-resolver] cstamas commented on pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#issuecomment-1312746749
There's much more to do, but am stopping here due https://issues.apache.org/jira/browse/MRESOLVER-295.
My plan is following:
* component should use component (even if trivial as Slawek noted) to perform IO. Reason: components can be swapped out
* split FileProcessor into PathProcessor (as name says, for files) and ChecksumProcessor (to read write checksums ONLY, to not mix concerns). This opens up the possibility to implement different ChecksumWriters (BSD/GNU/whatever), as currently we read BSD/GNU/resolver-native but write only resolver-native format.
* but, due module layout (SPI not depending on Utils, and both Impl and Connector depending on SPI but Connector not on Impl) this needs a bit more work: see how many implementations of FileProcessor we have across, exactly for this reason. So, I want to do something with it.
* direct use of static helper reduces testability, there is a nice test where file copy fails, that cannot be done if directly static helper used.
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1021290850
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -41,66 +42,55 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
- implements TrackingFileManager
+ implements TrackingFileManager
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );
@Override
public Properties read( File file )
{
- FileInputStream stream = null;
- try
+ Path filePath = file.toPath();
+ if ( Files.isRegularFile( filePath ) )
{
- if ( !file.exists() )
+ try ( InputStream stream = Files.newInputStream( filePath ) )
{
- return null;
+ Properties props = new Properties();
+ props.load( stream );
+ return props;
+ }
+ catch ( IOException e )
+ {
+ LOGGER.warn( "Failed to read tracking file '{}'", file, e );
Review Comment:
Agreed, but I guess it is out of the scope for this PR.
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020755138
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -489,7 +488,7 @@ protected void runTask()
}
}
}
- fileProcessor.move( tmp, file );
Review Comment:
Yes, is unused (connector was only one using it), hence I deprecated it (as it is in SPI that is public layer)
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #218: [MRESOLVER-290] Expand use of atomic file ops
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #218:
URL: https://github.com/apache/maven-resolver/pull/218#discussion_r1020498494
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java:
##########
@@ -44,9 +44,23 @@ private FileUtils()
*/
public interface TempFile extends Closeable
{
+ /**
+ * Returns the path of the created temp file.
+ */
Path getPath();
}
+ /**
+ * A collocated temporary file, that resides next to a "target" file, and is removed when closed.
Review Comment:
colocated and not collocated?!
--
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