-
Notifications
You must be signed in to change notification settings - Fork 462
Simplify conversions from java.nio.file.Path to File #5547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these changes are nice. There is some potential to move further away from String-types in some of the method return types and parameter types, to improve type safety.
core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/conf/HadoopCredentialProviderTest.java
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java
Outdated
Show resolved
Hide resolved
I've marked this as ready for review. I've gone through all the places that call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished reviewing, I'll finish on Monday. Posting my comments so far, most comments are repeats
It would be good to run the full set of tests for this PR as well.
core/src/main/java/org/apache/accumulo/core/spi/fs/PreferredVolumeChooser.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/file/rfile/GenerateSplitsTest.java
Outdated
Show resolved
Hide resolved
server/tserver/src/test/java/org/apache/accumulo/tserver/InMemoryMapTest.java
Show resolved
Hide resolved
server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/ImportExportIT.java
Outdated
Show resolved
Hide resolved
@kevinrr888 i pushed up 9891482 to address some of your comments. Regarding all the comments suggesting to |
Ah I see, feel free to resolve all those comments. |
testDir = baseDir.resolve(MiniAccumuloClusterTest.class.getName()).toFile(); | ||
FileUtils.deleteQuietly(testDir); | ||
assertTrue(testDir.mkdir()); | ||
Path testDir = tempDir.resolve(MiniAccumuloClusterTest.class.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be MiniAccumuloClusterClasspathTest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes LGTM so long as the full set of tests pass
No description provided.