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/11 15:29:56 UTC
[GitHub] [solr] madrob opened a new pull request #10: SOLR-15244 Switch File to Path
madrob opened a new pull request #10:
URL: https://github.com/apache/solr/pull/10
https://issues.apache.org/jira/browse/SOLR-15244
----------------------------------------------------------------
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
[GitHub] [solr] madrob merged pull request #10: SOLR-15244 Switch File to Path
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #10:
URL: https://github.com/apache/solr/pull/10
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #10: SOLR-15244 Switch File to Path
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #10:
URL: https://github.com/apache/solr/pull/10#discussion_r597967288
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStatesOps.java
##########
@@ -65,7 +65,13 @@ private static void persist(List<PerReplicaStates.Operation> operations, String
Op.create(path, null, zkClient.getZkACLProvider().getACLsToAdd(path), CreateMode.PERSISTENT) :
Op.delete(path, -1));
}
- zkClient.multi(ops, true);
+ try {
Review comment:
This snuck in?
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #10: SOLR-15244 Switch File to Path
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [solr] madrob commented on a change in pull request #10: SOLR-15244 Switch File to Path
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #10:
URL: https://github.com/apache/solr/pull/10#discussion_r596299172
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStatesOps.java
##########
@@ -65,13 +65,7 @@ private void persist(List<PerReplicaStates.Operation> operations, String znode,
Op.create(path, null, zkClient.getZkACLProvider().getACLsToAdd(path), CreateMode.PERSISTENT) :
Op.delete(path, -1));
}
- try {
- zkClient.multi(ops, true);
- } catch (KeeperException e) {
- log.error("Multi-op exception: {}", zkClient.getChildren(znode, null, true));
- throw e;
- }
-
+ zkClient.multi(ops, true);
Review comment:
this snuck in and shouldn't be here, will remove before next iteration.
##########
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:
Oh, uh, I didn't even notice. Thanks for pointing this out, I'll take a closer look.
##########
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:
yes
##########
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:
Yea, this change is still fine, the old code is doing a bunch of extra conversions between Hadoop Path, URI, and String, and back and forth.
----------------------------------------------------------------
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