HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO … #1450

Sanne · 2017-06-14T17:00:55Z

…APIs

A bit of a mindless, boring translation task.. but IMO the code looks much more readable now, and I could get rid of several deprecated methods.

https://hibernate.atlassian.net/browse/HSEARCH-2775

yrodiere

Nice cleanup :)
A few comments below; they may look like nitpicking for some, but if we don't talk about this now, I don't think we ever will, so...
Note some problems may be very important, most notably unclosed streams.

yrodiere · 2017-06-15T07:24:08Z

engine/src/main/java/org/hibernate/search/store/impl/FSDirectoryProvider.java

-			return false;
-		}
-		return indexName.equals( ( (FSDirectoryProvider) obj ).indexName );
+		throw new AssertionFailure( "this type can not be compared reliably" );


You could also simply throw an exception when this method is called before initialization, which would be a bit easier to callers.
But why do we need a custom implementation of equals on this class anyway?

Ok, I just saw how unnecessarily complex the other implementations are. You're right, let's just forbid its use.

We used to need this, so for that reason they had "reasonable" implementations.

That requirement is gone - since many years a I suspect - and since it's damn complex to do it "right" I prefer to introduce the assertion to make sure we never rely on it.

yrodiere · 2017-06-15T07:30:55Z

engine/src/main/java/org/hibernate/search/store/impl/FSMasterDirectoryProvider.java

 				try {
-					new File( destination, CURRENT_DIR_NAME[index] ).createNewFile();
+					Files.delete( destination.resolve( CURRENT_DIR_NAME[oldIndex] ) );


So you don't want to use log.unableToRemovePreviousMarket anymore... ?
(By the way, that method should be renamed unableToRemovePreviousMarke*r*)

that's right. The File API doesn't throw an error when it fails, it would just return false.
The NIO API throws appropriate exceptions.

yrodiere · 2017-06-15T07:35:26Z

engine/src/main/java/org/hibernate/search/store/impl/FSSlaveDirectoryProvider.java

 				}

 				// copy is required
 				int oldIndex = current;
 				int index = oldIndex == 1 ? 2 : 1;
-				File destinationFile = new File( destination, Integer.valueOf( index ).toString() );
+				Path destinationFile = destination.resolve( Integer.valueOf( index ).toString() );
 				try {
 					log.tracef( "Copying %s into %s", sourceFile, destinationFile );
 					FileHelper.synchronize( sourceFile, destinationFile, true, copyChunkSize );


Seems like you forgot some occurrences of File below (new File( indexName, "current" + oldIndex ).delete() for instance)

yrodiere · 2017-06-15T07:38:17Z

engine/src/main/java/org/hibernate/search/store/spi/LockFactoryCreator.java

@@ -31,5 +32,5 @@
 	 * @return the lock factory as configured, or a factory adhering to the "simple" strategy in case of configuration
 	 * errors or as a default.
 	 */
-	LockFactory createLockFactory(File indexDir, Properties dirConfiguration);
+	LockFactory createLockFactory(Path indexDir, Properties dirConfiguration);


There's also a reference to File in the javadoc above ({@link File}).

yrodiere · 2017-06-15T07:42:21Z

engine/src/main/java/org/hibernate/search/util/impl/FileHelper.java

+				tryDelete( destination );
+			}
+			final File sourceFile = source.toFile();
+			final File destinationFile = destination.toFile();


Any reason why you don't want to convert copyFile to use Path too? It would avoid such conversions, and it would make the main code (tests excluded) entirely File-free!

ok, but I just wanted to remove a File reference from a deprecated SPI and look what it got me into :)

yrodiere · 2017-06-15T08:06:25Z

engine/src/main/java/org/hibernate/search/util/impl/FileHelper.java

+			Files.createDirectories( destination );
+			Set<Path> sources = Files.list( source ).collect( Collectors.toSet() );
+			Set<String> sourceFilenames = sources.stream().map( v -> v.getFileName().toString() ).collect( Collectors.toSet() );
+			Set<Path> dests = Files.list( destination ).collect( Collectors.toSet() );


I'm pretty sure you don't need this set. Just using Files.list( destination ).map instead of dests.stream().map when building destFilenames below would work equally well.

And while we're at it, I don't think you need to collect destination files at all. This would work just fine for deletion, and destination files are not used anywhere else:

//delete files not present in source try ( DirectoryStream<Path> dests = Files.newDirectoryStream( source ) ) { for ( Path dest : dests ) { if ( !sourceFilenames.contains( dest.getFileName().toString() ) ) { delete( dest ); } } }

You could also avoid collecting source file names and just resolve each dest file name against the source, but I'm not sure there's a benefit in that.

yrodiere · 2017-06-15T08:22:45Z

engine/src/main/java/org/hibernate/search/util/impl/FileHelper.java

-			String[] dests = destination.list();
+		if ( Files.isDirectory( source ) ) {
+			Files.createDirectories( destination );
+			Set<Path> sources = Files.list( source ).collect( Collectors.toSet() );


I must admit I'm not very familiar with nio, but shouldn't we explicitly close the streams? (stream.close())

From the javadoc of Files.list:

* <p> The returned stream encapsulates a {@link DirectoryStream}. * If timely disposal of file system resources is required, the * {@code try}-with-resources construct should be used to ensure that the * stream's {@link Stream#close close} method is invoked after the stream * operations are completed.

Good catch. I assumed this was handled by the Stream/Collector framework but apparently it will only do that if you flatmap the stream (?!)

@Sanne Unless I'm mistaken, you didn't address this... ?

@yrodiere you're right. How embarrassing. There are two blocks of code which are very similar, I fixed one and missed the other.

yrodiere · 2017-06-15T08:33:23Z

engine/src/test/java/org/hibernate/search/test/util/impl/FileHelperTest.java

-	private void writeDummyDataToFile(File file) throws IOException {
-		FileOutputStream os = new FileOutputStream( file, true );
+	private void writeDummyDataToFile(Path file) throws IOException {
+		FileOutputStream os = new FileOutputStream( file.toFile(), true );


I think the nio way would be: Files.newOutputStream( file, StandardOpenOption.APPEND );

yrodiere · 2017-06-15T08:38:06Z

engine/src/main/java/org/hibernate/search/store/impl/FSMasterDirectoryProvider.java

-		log.debugf( "Index directory: %s", indexDir.getPath() );
+		sourceDir = DirectoryProviderHelper.getSourceDirectoryPath( directoryProviderName, properties, true );
+		log.debugf( "Source directory: %s", sourceDir );
+		indexDir = DirectoryHelper.getVerifiedIndexPath( directoryProviderName, properties, true );


You may want to normalize the path at some point (indexDir = indexDir.normalize()), since previously we used indexDir.getCanonicalPath() to set indexName. Or maybe it's not important now that we don't use it for equals anymore?

I think it's no longer crucial, but yes I'll do it. Will at least look better when logging the dir..

yrodiere · 2017-06-15T08:38:40Z

engine/src/main/java/org/hibernate/search/store/impl/FSSlaveDirectoryProvider.java

-		}
+		sourceIndexDir = DirectoryProviderHelper.getSourceDirectoryPath( directoryProviderName, properties, false );
+		log.debugf( "Source directory: %s", sourceIndexDir );
+		indexDir = DirectoryHelper.getVerifiedIndexPath( directoryProviderName, properties, true );


Same remark as in FSMasterDirectoryProvider.java about normalizing indexDir.

Sanne · 2017-06-15T12:52:53Z

rebased and updated

…APIs

…tion property

Sanne · 2017-06-15T15:41:05Z

Fixed it an added a bonus commit for:

- https://hibernate.atlassian.net/browse/HSEARCH-2783

yrodiere · 2017-06-15T17:29:12Z

Merged! Thanks

Sanne added the Ready for review label Jun 14, 2017

yrodiere self-assigned this Jun 15, 2017

yrodiere requested changes Jun 15, 2017

View reviewed changes

yrodiere added Changes required and removed Ready for review labels Jun 15, 2017

Sanne force-pushed the HSEARCH-2775 branch from 44b34af to 3f42db6 Compare June 15, 2017 12:52

Sanne added Ready for review and removed Changes required labels Jun 15, 2017

yrodiere added Changes required and removed Ready for review labels Jun 15, 2017

Sanne added 2 commits June 15, 2017 16:09

HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO …

ceaf592

…APIs

HSEARCH-2783 Deprecate no longer useful buffer_size_on_copy configura…

e339f2e

…tion property

Sanne force-pushed the HSEARCH-2775 branch from 3f42db6 to e339f2e Compare June 15, 2017 15:40

Sanne added Ready for review and removed Changes required labels Jun 15, 2017

yrodiere approved these changes Jun 15, 2017

View reviewed changes

yrodiere merged commit e339f2e into hibernate:master Jun 15, 2017

Sanne deleted the HSEARCH-2775 branch June 15, 2017 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO … #1450

HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO … #1450

Sanne commented Jun 14, 2017

yrodiere left a comment

yrodiere Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

yrodiere Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

yrodiere Jun 15, 2017

Sanne Jun 15, 2017

yrodiere Jun 15, 2017

Sanne commented Jun 15, 2017

Sanne commented Jun 15, 2017

yrodiere commented Jun 15, 2017

HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO … #1450

HSEARCH-2775 Migrate internal file helpers and SPIs from File to NIO … #1450

Conversation

Sanne commented Jun 14, 2017

yrodiere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanne commented Jun 15, 2017

Sanne commented Jun 15, 2017

yrodiere commented Jun 15, 2017