keith-turner commented on code in PR #3509:
URL: https://github.com/apache/accumulo/pull/3509#discussion_r1237023835
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java:
##########
@@ -49,33 +51,54 @@ public class TabletFile implements Comparable<TabletFile> {
private static final Logger log = LoggerFactory.getLogger(TabletFile.class);
+ private static final String HDFS_TABLES_DIR_NAME =
HDFS_TABLES_DIR.substring(1);
+
/**
* Construct new tablet file using a Path. Used in the case where we had to
use Path object to
* qualify an absolute path or create a new file.
*/
public TabletFile(Path metaPath) {
this.metaPath = Objects.requireNonNull(metaPath);
- String errorMsg = "Missing or invalid part of tablet file metadata entry:
" + metaPath;
+ final String errorMsg = "Missing or invalid part of tablet file metadata
entry: " + metaPath;
log.trace("Parsing TabletFile from {}", metaPath);
- // use Path object to step backwards from the filename through all the
parts
- this.fileName = metaPath.getName();
- ValidationUtil.validateFileName(fileName);
+ // File name construct: <volume>/<tablePath>/<tableId>/<tablet>/<file>
+ // Example: hdfs://namenode:9020/accumulo/tables/1/default_tablet/F00001.rf
+ final URI uri = this.metaPath.toUri();
+ final String path = uri.getPath();
+ final String[] parts = path.split("/");
+ final int numParts = parts.length;
- Path tabletDirPath = Objects.requireNonNull(metaPath.getParent(),
errorMsg);
+ if (numParts < 4) {
+ throw new IllegalArgumentException(errorMsg);
+ }
- Path tableIdPath = Objects.requireNonNull(tabletDirPath.getParent(),
errorMsg);
- var id = tableIdPath.getName();
+ this.fileName = parts[numParts - 1];
+ final String tabletDirectory = parts[numParts - 2];
+ final String tableId = parts[numParts - 3];
+ final String tablesPath = parts[numParts - 4];
+ if (StringUtils.isBlank(fileName) || StringUtils.isBlank(tabletDirectory)
+ || StringUtils.isBlank(tableId) || StringUtils.isBlank(tablesPath)) {
+ throw new IllegalArgumentException(errorMsg);
+ }
+ ValidationUtil.validateFileName(fileName);
+ Preconditions.checkArgument(tablesPath.equals(HDFS_TABLES_DIR_NAME),
+ "tables directory name is not " + HDFS_TABLES_DIR_NAME + ", is " +
tablesPath);
- Path tablePath = Objects.requireNonNull(tableIdPath.getParent(), errorMsg);
- String tpString = "/" + tablePath.getName();
- Preconditions.checkArgument(tpString.equals(HDFS_TABLES_DIR), errorMsg);
+ // determine where file path starts, the rest is the volume
+ final String filePath =
+ HDFS_TABLES_DIR + "/" + tableId + "/" + tabletDirectory + "/" +
this.fileName;
Review Comment:
I think this is the exact same thing that is being computed for
normalizedPath later in the code. So could move that computation here instead.
Will need to adjust the rest of the code.
```suggestion
this.tabletDir = new TabletDirectory(volume, TableId.of(tableId),
tabletDirectory);
this.normalizedPath = tabletDir.getNormalizedPath() + "/" + fileName;
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletFile.java:
##########
@@ -49,33 +51,54 @@ public class TabletFile implements Comparable<TabletFile> {
private static final Logger log = LoggerFactory.getLogger(TabletFile.class);
+ private static final String HDFS_TABLES_DIR_NAME =
HDFS_TABLES_DIR.substring(1);
+
/**
* Construct new tablet file using a Path. Used in the case where we had to
use Path object to
* qualify an absolute path or create a new file.
*/
public TabletFile(Path metaPath) {
this.metaPath = Objects.requireNonNull(metaPath);
- String errorMsg = "Missing or invalid part of tablet file metadata entry:
" + metaPath;
+ final String errorMsg = "Missing or invalid part of tablet file metadata
entry: " + metaPath;
log.trace("Parsing TabletFile from {}", metaPath);
- // use Path object to step backwards from the filename through all the
parts
- this.fileName = metaPath.getName();
- ValidationUtil.validateFileName(fileName);
+ // File name construct: <volume>/<tablePath>/<tableId>/<tablet>/<file>
+ // Example: hdfs://namenode:9020/accumulo/tables/1/default_tablet/F00001.rf
+ final URI uri = this.metaPath.toUri();
+ final String path = uri.getPath();
+ final String[] parts = path.split("/");
+ final int numParts = parts.length;
- Path tabletDirPath = Objects.requireNonNull(metaPath.getParent(),
errorMsg);
+ if (numParts < 4) {
+ throw new IllegalArgumentException(errorMsg);
+ }
- Path tableIdPath = Objects.requireNonNull(tabletDirPath.getParent(),
errorMsg);
- var id = tableIdPath.getName();
+ this.fileName = parts[numParts - 1];
+ final String tabletDirectory = parts[numParts - 2];
+ final String tableId = parts[numParts - 3];
+ final String tablesPath = parts[numParts - 4];
+ if (StringUtils.isBlank(fileName) || StringUtils.isBlank(tabletDirectory)
+ || StringUtils.isBlank(tableId) || StringUtils.isBlank(tablesPath)) {
+ throw new IllegalArgumentException(errorMsg);
+ }
+ ValidationUtil.validateFileName(fileName);
+ Preconditions.checkArgument(tablesPath.equals(HDFS_TABLES_DIR_NAME),
+ "tables directory name is not " + HDFS_TABLES_DIR_NAME + ", is " +
tablesPath);
- Path tablePath = Objects.requireNonNull(tableIdPath.getParent(), errorMsg);
- String tpString = "/" + tablePath.getName();
- Preconditions.checkArgument(tpString.equals(HDFS_TABLES_DIR), errorMsg);
+ // determine where file path starts, the rest is the volume
+ final String filePath =
+ HDFS_TABLES_DIR + "/" + tableId + "/" + tabletDirectory + "/" +
this.fileName;
+ final String uriString = uri.toString();
+ int idx = uriString.indexOf(filePath);
- Path volumePath = Objects.requireNonNull(tablePath.getParent(), errorMsg);
- Preconditions.checkArgument(volumePath.toUri().getScheme() != null,
errorMsg);
- var volume = volumePath.toString();
+ if (idx == -1) {
+ throw new IllegalArgumentException(errorMsg);
+ }
- this.tabletDir = new TabletDirectory(volume, TableId.of(id),
tabletDirPath.getName());
+ // The volume is the remaining part of the path.
+ final String volume = uriString.substring(0, idx);
+ Preconditions.checkArgument(URI.create(volume).getScheme() != null,
errorMsg);
Review Comment:
Seems like we can avoid creating another URI object, seems like the volumes
scheme should be the same as the scheme on the original URI. Could also move
this to the beginning of the method.
```suggestion
Preconditions.checkArgument(uri.getScheme() != null, errorMsg);
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
