You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/03/12 04:58:31 UTC
[GitHub] [solr] dsmiley commented on a change in pull request #10: SOLR-15244 Switch File to Path
dsmiley commented on a change in pull request #10:
URL: https://github.com/apache/solr/pull/10#discussion_r592904003
##########
File path: solr/core/src/java/org/apache/solr/cloud/CloudUtil.java
##########
@@ -125,7 +125,7 @@ public static boolean replicaExists(ClusterState clusterState, String collection
public static String unifiedResourcePath(SolrResourceLoader loader) {
return (loader instanceof ZkSolrResourceLoader) ?
((ZkSolrResourceLoader) loader).getConfigSetZkPath() + "/" :
- loader.getConfigDir() + File.separator;
+ loader.getConfigPath().toString() + File.separator;
Review comment:
Referencing File.separator seems wrong here; just use '/'. toString() is automatic as well.
##########
File path: solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java
##########
@@ -176,29 +176,24 @@ public void stop() {
/**
* Parse a ZooKeeper configuration file
* @param path the patch of the configuration file
+ * @throws IllegalArgumentException if a config file does not exist at the given path
* @throws ConfigException error processing configuration
*/
public static Properties getProperties(String path) throws ConfigException {
- File configFile = new File(path);
-
- log.info("Reading configuration from: {}", configFile);
+ Path configPath = Path.of(path);
+ log.info("Reading configuration from: {}", configPath);
try {
- if (!configFile.exists()) {
- throw new IllegalArgumentException(configFile.toString()
+ if (!Files.exists(configPath)) {
+ throw new IllegalArgumentException(configPath.toString()
+ " file is missing");
}
- Properties cfg = new Properties();
- FileInputStream in = new FileInputStream(configFile);
- try {
- cfg.load(new InputStreamReader(in, StandardCharsets.UTF_8));
- } finally {
- in.close();
+ try (Reader reader = Files.newBufferedReader(configPath)) {
Review comment:
love it
##########
File path: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
##########
@@ -1521,7 +1520,7 @@ public DirectoryFileStream(SolrParams solrParams) {
sOffset = params.get(OFFSET);
sLen = params.get(LEN);
- compress = params.get(COMPRESSION);
+ compress = Boolean.parseBoolean(params.get(COMPRESSION));
Review comment:
this snuck in here but okay
##########
File path: solr/test-framework/src/java/org/apache/solr/core/MockFSDirectoryFactory.java
##########
@@ -65,7 +64,7 @@ private Directory reduce(Directory dir) {
if (dir instanceof NRTCachingDirectory) {
cdir = ((NRTCachingDirectory)dir).getDelegate();
}
- if (cdir instanceof TrackingDirectoryWrapper) {
+ if (dir instanceof TrackingDirectoryWrapper) {
Review comment:
uh... is this change right?
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -582,29 +581,26 @@ private static String _getMetapath(String path) {
*/
public static void _persistToFile(Path solrHome, String path, ByteBuffer data, ByteBuffer meta) throws IOException {
Path realpath = _getRealPath(path, solrHome);
- File file = realpath.toFile();
- File parent = file.getParentFile();
- if (!parent.exists()) {
- parent.mkdirs();
+ Path parent = realpath.getParent();
+ if (!Files.exists(parent)) {
+ Files.createDirectories(realpath.getParent());
Review comment:
again; probably don't need to check that it exists first
##########
File path: solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java
##########
@@ -61,8 +60,7 @@ public void init(NamedList args) {
@Override
protected Directory create(String path, LockFactory lockFactory, DirContext dirContext) throws IOException {
- // we pass NoLockFactory, because the real lock factory is set later by injectLockFactory:
Review comment:
was this comment obsolete?
##########
File path: solr/core/src/java/org/apache/solr/core/snapshots/SolrSnapshotsTool.java
##########
@@ -183,26 +180,29 @@ public void describeSnapshot(String collectionName, String snapshotName) {
}
}
- public Map<String, List<String>> getIndexFilesPathForSnapshot(String collectionName, String snapshotName, Optional<String> pathPrefix)
+ /**
+ * @param pathPrefix optional
+ */
+ public Map<String, List<String>> getIndexFilesPathForSnapshot(String collectionName, String snapshotName, String pathPrefix)
throws SolrServerException, IOException {
Map<String, List<String>> result = new HashMap<>();
Collection<CollectionSnapshotMetaData> snaps = listCollectionSnapshots(collectionName);
- Optional<CollectionSnapshotMetaData> meta = Optional.empty();
+ CollectionSnapshotMetaData meta = null;
Review comment:
I'm not a fan of Optional either
##########
File path: solr/core/src/java/org/apache/solr/core/snapshots/SolrSnapshotsTool.java
##########
@@ -244,20 +243,16 @@ public void describeSnapshot(String collectionName, String snapshotName) {
return result;
}
- public void buildCopyListings(String collectionName, String snapshotName, String localFsPath, Optional<String> pathPrefix)
+ /**
+ * @param pathPrefix optional
+ */
+ public void buildCopyListings(String collectionName, String snapshotName, String localFsPath, String pathPrefix)
throws SolrServerException, IOException {
Map<String, List<String>> paths = getIndexFilesPathForSnapshot(collectionName, snapshotName, pathPrefix);
for (Map.Entry<String,List<String>> entry : paths.entrySet()) {
- StringBuilder filesBuilder = new StringBuilder();
- for (String filePath : entry.getValue()) {
- filesBuilder.append(filePath);
- filesBuilder.append("\n");
- }
-
- String files = filesBuilder.toString().trim();
- try (Writer w = new OutputStreamWriter(new FileOutputStream(new File(localFsPath, entry.getKey())), StandardCharsets.UTF_8)) {
- w.write(files);
- }
+ // TODO: this used to trim - check if that's needed
+ // Using Paths.get instead of Path.of because of conflict with o.a.hadoop.fs.Path
+ Files.write(Paths.get(localFsPath, entry.getKey()), entry.getValue());
Review comment:
I learned something new today; thanks!
##########
File path: solr/core/src/java/org/apache/solr/core/snapshots/SolrSnapshotsTool.java
##########
@@ -226,10 +226,9 @@ public void describeSnapshot(String collectionName, String snapshotName) {
}
String indexDirPath = coreSnap.getIndexDirPath();
- if (pathPrefix.isPresent()) {
+ if (pathPrefix != null) {
// If the path prefix is specified, rebuild the path to the index directory.
- Path t = new Path(coreSnap.getIndexDirPath());
- indexDirPath = (new Path(pathPrefix.get(), t.toUri().getPath())).toString();
+ indexDirPath = new Path(pathPrefix, coreSnap.getIndexDirPath()).toString();
Review comment:
This is referring to Hadoop APIs! `org.apache.hadoop.fs.Path` Ugh! Could it have been intentional and thus is there risk your change actually breaks something?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -915,27 +921,22 @@ private PackageListeningClassLoader createSchemaLoader() {
public static void persistConfLocally(SolrResourceLoader loader, String resourceName, byte[] content) {
// Persist locally
- File confFile = new File(loader.getConfigDir(), resourceName);
+ Path confFile = loader.getConfigPath().resolve(resourceName);
try {
- File parentDir = confFile.getParentFile();
- if (!parentDir.isDirectory()) {
- if (!parentDir.mkdirs()) {
- final String msg = "Can't create managed schema directory " + parentDir.getAbsolutePath();
- log.error(msg);
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg);
- }
- }
- try (OutputStream out = new FileOutputStream(confFile);) {
- out.write(content);
+ Path parentDir = confFile.getParent();
+ if (!Files.isDirectory(parentDir)) {
+ Files.createDirectories(parentDir);
Review comment:
according to javadocs, createDirectories doesn't throw if parentDir already exists so you needn't conditionally check.
----------------------------------------------------------------
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