Repository navigation
Conversation
This change modifies the remaining code that allows for relative RFile and WAL references. The RFiles and WALs that are created today are done with absolute paths, but some of the existing code does not enforce or check that.
| + "Setting this to zero will make bloom filters load in the foreground.", | ||
| "1.3.5"), | ||
| TSERV_MEMDUMP_DIR("tserver.dir.memdump", "/tmp", PropertyType.PATH, """ | ||
| TSERV_MEMDUMP_DIR("tserver.dir.memdump", "file:/tmp", PropertyType.ABSOLUTEPATH, """ |
There was a problem hiding this comment.
Why make this have to be a URI? Its a local file system path and will always be file:/
| * hdfs://foo:6000/accumulo/tables/4/t0/F000.rf -> 4/t0/F000.rf. | ||
| */ | ||
| private String makeRelativeFileReference(String path) { | ||
| Path referencePath = new Path(path); |
There was a problem hiding this comment.
Had a vague memory of performance problems w/ Path and did some digging and found #3509. So it seems calling getParent() on path is slow. Was wondering if the code below could use the path object and call getParent() instead of doing string parsing, but I guess it should not.
There was a problem hiding this comment.
A cheap string check would preserve the performance i think. but not sure if gc cpu is a concern
| upgradeDataFileCF(key, value, update); | ||
| } else if (family.equals(LogColumnFamily.NAME)) { | ||
| try { | ||
| LogEntry.fromMetaWalEntry(entry); |
There was a problem hiding this comment.
Seems like this is not actually changing anything in the metadata table, its only validating. There are many other columns in the metadata table that we do not validate on upgrade, so this seems like new functionality in upgrade. We could generalize this to upgrade the metadata table and then the upgrade could do a second scan creating tablet metadata object which would do this validation and more. Not something for this PR, but validating the metadata table on upgrade seems like an interesting concept.
There was a problem hiding this comment.
I'm not sure that I want to leave this change here. This PR, and some others, are precursors to a pluggable WAL SPI. What I really want is a check that runs pre-upgrade that confirms that there are no tablets that need recovery. The check currently has to go here because the --prepare step is done after the instance is shut down. I guess another option would be to move this to the --prepare step in the 2.x or 3.1 version and modify the prepare step to check the root tablet, metadata tablet, and user tables to ensure that logs are not present in the tablet metadata.
There was a problem hiding this comment.
#6623 includes the changes to the upgrade code that I was thinking about
| String[] tokens = path.split("/"); | ||
| String[] suffix = new String[4]; | ||
| int suffixLen = 0; | ||
| for (int i = tokens.length - 1; i >= 0 && suffixLen < suffix.length; i--) { |
There was a problem hiding this comment.
This seems much more efficient than the existing makeRelative method which uses a stream. The GC can burn a lot of CPU churning through file refs and in the past CPU has been a bottle neck.
|
The ITs still build relative paths no? Should those receive the same treatment? Unit tests were converted to relatives... |
| throw new IllegalArgumentException(badUuidMsg); | ||
| } | ||
|
|
||
| Path logPath = new Path(path); |
There was a problem hiding this comment.
Logreader passes a user supplied path. A user passing a relative directory path would now get “The path must be an absolute URI” rather than it resolving against the working directory. Consider fs.makequalified(...) or if the caller change is acceptable then this is a non-issue.
|
|
||
| this.memDumpDir = config.get(Property.TSERV_MEMDUMP_DIR); | ||
| String memDumpDir = config.getPath(Property.TSERV_MEMDUMP_DIR); | ||
| Preconditions.checkArgument( |
There was a problem hiding this comment.
Isn't this already checked by SystemPropUtil or PropUtil ( i forget ).
There was a problem hiding this comment.
AccumuloConfiguration.getPath can return null at https://github.com/apache/accumulo/blob/main/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L399.
Address in ed94024 |
|
Sunny ITs passed |
This change modifies the remaining code that allows for relative RFile and WAL references. The RFiles and WALs that are created today are done with absolute paths, but some of the existing code does not enforce or check that.