Add code to replace relative paths on upgrade. #1397 #1461
Conversation
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
| String metaEntry = key.getColumnQualifier().toString(); | ||
| Path relPath = resolveRelativePath(metaEntry, key); | ||
| Path absPath = new Path(volumeProp, relPath); | ||
| if (fs.exists(absPath)) { |
There was a problem hiding this comment.
Why do existence check again?
There was a problem hiding this comment.
I was thinking the 2 static methods could be called by a command line utility to give admins the chance to resolve the paths before an upgrade. I can remove the second check as long as we make sure the utility calls the other method first.
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/dataImpl/KeyExtent.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I can't wait to have this done. I noticed there was no description in the commit log message, or PR description box. It would be helpful if the log message when this is eventually merged have a few more details about what the change accomplishes.
For example:
Resolve and replace all relative paths found in metadata tables with absolute
paths during upgrade. Absolute paths are resolved by prefixing relative paths
with a volume configured by the user in a new configuration property created
specifically for this purpose. If any relative paths are found and this property
is not configured, or if any resolved absolute path does not correspond to a
file that actually exists, the upgrade step fails and aborts without making changes.
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/UpgradeRelativePathsIT.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/UpgradeRelativePathsIT.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/UpgradeRelativePathsIT.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/UpgradeRelativePathsIT.java
Outdated
Show resolved
Hide resolved
| if (!filePath.toString().contains(":")) { | ||
| // found relative paths so verify the property used to build the absolute paths | ||
| hasRelatives = true; | ||
| if (upgradeProperty == null || upgradeProperty.isBlank()) { |
There was a problem hiding this comment.
Will the upgradeProperty be normalized by the new Path code? If not, it might be useful to error out on things like having multiple // (except if it immediately follows the first :, as in hdfs://) or having any ../ present in the string. If Path normalizes this, then this is less important.
There was a problem hiding this comment.
I believe so... after this check, I create the path: Path absPath = new Path(upgradeProperty, relPath);
There was a problem hiding this comment.
That might just do a simple concatenation with the appropriate file separator. It might not do normalization.
There was a problem hiding this comment.
Looking at the source, there is some normalization done by Path using the java URI object here. And as far as I can tell that source is the same as the source I downloaded for 3.1.2: http://archive.apache.org/dist/hadoop/common/hadoop-3.1.2/hadoop-3.1.2-src.tar.gz
There was a problem hiding this comment.
I also confirmed that extra intermediate / get removed by this normalization, with my last update adding an extra erroneously to one of the relative paths:
tables//1b/default_tablet/A000001c.rf becomes tables/1b/default_tablet/A000001c.rf
It won't hurt to add some erroneous paths to the unit test though, now that I have one.
There was a problem hiding this comment.
It won't hurt to add some erroneous paths to the unit test though, now that I have one.
Agreed.
server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Keith Turner <kturner@apache.org>
milleruntime
force-pushed
the
upgrade-relative
branch
from
b7ee307
to
95174ea
Compare
|
I think this is ready to go |
No description provided.