You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/01/21 20:57:59 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1479: Clean up
relative path code
milleruntime opened a new pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479
* Drop methods in VolumeManager
* Create TabletFileUtil to do path validation
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369730135
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##########
@@ -3217,7 +3216,7 @@ public void recover(VolumeManager fs, KeyExtent extent, List<LogEntry> logEntrie
Collections.sort(sorted, (e1, e2) -> (int) (e1.timestamp - e2.timestamp));
for (LogEntry entry : sorted) {
Path recovery = null;
- Path finished = RecoveryPath.getRecoveryPath(fs.getFullPath(FileType.WAL, entry.filename));
+ Path finished = RecoveryPath.getRecoveryPath(new Path(entry.filename));
Review comment:
> so I think we should do it when the LogEntry is created
I like that approach. We do need to avoid constantly validating the same data over and over in the code. I think the goal should be that data is only validated after its read from a persistent store and no where else.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369603076
##########
File path: server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
##########
@@ -319,9 +319,9 @@ public void delete(SortedMap<String,String> confirmedDeletes) throws TableNotFou
// of deleting something that should not be deleted. Must not change value of delete
// variable because thats whats stored in metadata table.
log.debug("Volume replaced {} -> {}", delete, switchedDelete);
- fullPath = fs.getFullPath(FileType.TABLE, switchedDelete);
+ fullPath = new Path(switchedDelete);
Review comment:
Should these be validated?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369606459
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/TabletData.java
##########
@@ -58,19 +58,18 @@ public TabletData(KeyExtent extent, VolumeManager fs, TabletMetadata meta) {
this.flushID = meta.getFlushId().orElse(-1);
this.directoryName = meta.getDirName();
this.logEntries.addAll(meta.getLogs());
- meta.getScans().forEach(tabletFile -> scanFiles
- .add(new FileRef(fs, tabletFile.getMetadataEntry(), meta.getTableId())));
+ meta.getScans()
+ .forEach(tabletFile -> scanFiles.add(new FileRef(tabletFile.getMetadataEntry())));
Review comment:
Would probably be useful to have a FileRef constructor that takes a TabletFile type. This way its always converted easily and consistently.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369667221
##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -419,66 +407,6 @@ public Path matchingFileSystem(Path source, String[] options) {
return null;
}
- @Override
- public Path getFullPath(TableId tableId, String path) {
Review comment:
I think ```getVolumeByPath``` may have to be done in another change, related to the DEFAULT volume code. That method returns the DEFAULT volume if it detects a relative path, which I could remove but I wanted to look more into whether the DEFAULT volume can be removed with the other legacy properties.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369763823
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
It's not purely formatting. I would argue that the overloaded method would help centralize the logic related to validation by simplifying calling code, and that, in turn, makes it easier to swap out later (because working with concrete types rather than Strings helps you create replacement code that satisfies all your caller use cases). Also, avoiding the `toString()` calls and using the concrete types instead, can help find a lot of situations where code is being unnecessarily converted back and forth to/from String.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369574231
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
I 100% wrote it this way for nice formatting as any other way wraps the line of code. I think your way make sense logically though.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369749197
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
LOL, I wasn't thinking about formatting. Now, I'm not so sure. It might fit if `validate` were overloaded to take a `Text` so you didn't have to do `toString()`? Whatever you think best.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime merged pull request #1479: Clean up
relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369746902
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata;
+
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Utility class for validation of metadata tablet files.
+ */
+public class TabletFileUtil {
+
+ /**
+ * Validate if string is a valid path. Return same string or throw exception if not valid. This
+ * was added to facilitate more use of TabletFile over String but this puts the validation in one
+ * location in the case where TabletFile can't be used.
+ */
+ public static String validate(String path) {
+ Path p = new Path(path);
+ if (p.toUri().getScheme() == null) {
+ throw new IllegalArgumentException("Invalid path provided, no scheme in " + path);
+ }
+ return path;
Review comment:
Not sure. Probably Path, but it depends on caller needs. I haven't investigated.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369754760
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
Ah man... I don't like overloaded methods to begin with, let alone adding one purely for formatting haha. I'll see how your suggestion fits and if its ugly ill just keep what I had.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369244035
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
The old code did a kind of normalization here against the volume manager's filesystem. I would hope that by the time it gets here, all callers would be providing a normalized string, but care should be taken if the parameter is provided from user input, that we normalize it first. I didn't track down all the callers to check this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369641045
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
Which object do you think would be better? The issue is that we have some use of Path, ByteSequence and String. Calls to the ```createDeleteMutation()``` method are particularly hairy as there are a few spots where we parse the string and do string manipulation:
[gc](https://github.com/apache/accumulo/blob/3fd5cad92f9b63ac19e4466f3f2d5237b905262c/server/base/src/main/java/org/apache/accumulo/server/gc/GcVolumeUtil.java#L36)
[upgrader](https://github.com/apache/accumulo/blob/3276e788e8fa7ebf9c5f1fa4f37f49bad4eaeb3a/server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java#L434)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369742597
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
That being said, I think a lot of that can be done in other PRs while the goal of this one was to remove some of the relative path code that was no longer needed in VolumeManager
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369245216
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -278,15 +278,12 @@ public static void addDeleteEntries(KeyExtent extent, Set<FileRef> datafilesToDe
// TODO could use batch writer,would need to handle failure and retry like update does -
// ACCUMULO-1294
for (FileRef pathToRemove : datafilesToDelete) {
- update(context,
- ServerAmpleImpl.createDeleteMutation(context, tableId, pathToRemove.path().toString()),
- extent);
+ update(context, ServerAmpleImpl.createDeleteMutation(pathToRemove.path().toString()), extent);
Review comment:
An impl method could take the more precise type, rather than the `toString()` version, without affecting the API.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369782419
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata;
+
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Utility class for validation of metadata tablet files.
+ */
+public class TabletFileUtil {
+
+ /**
+ * Validate if string is a valid path. Return same string or throw exception if not valid. This
+ * was added to facilitate more use of TabletFile over String but this puts the validation in one
+ * location in the case where TabletFile can't be used.
+ */
+ public static String validate(String path) {
+ Path p = new Path(path);
+ if (p.toUri().getScheme() == null) {
+ throw new IllegalArgumentException("Invalid path provided, no scheme in " + path);
+ }
+ return path;
Review comment:
I think these could also get replaced with TabletFile or Ample as well. So I will just leave as String. If we can replace them all, this class will go away
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369724241
##########
File path: server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
##########
@@ -319,9 +319,9 @@ public void delete(SortedMap<String,String> confirmedDeletes) throws TableNotFou
// of deleting something that should not be deleted. Must not change value of delete
// variable because thats whats stored in metadata table.
log.debug("Volume replaced {} -> {}", delete, switchedDelete);
- fullPath = fs.getFullPath(FileType.TABLE, switchedDelete);
+ fullPath = new Path(switchedDelete);
Review comment:
Looking at the code, I can't figure out if these are delete markers or the files to delete...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on issue #1479: Clean up relative
path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on issue #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#issuecomment-578210504
I am going to merge this since most of the comments pertained to future work.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369242513
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata;
+
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Utility class for validation of metadata tablet files.
+ */
+public class TabletFileUtil {
+
+ /**
+ * Validate if string is a valid path. Return same string or throw exception if not valid. This
+ * was added to facilitate more use of TabletFile over String but this puts the validation in one
+ * location in the case where TabletFile can't be used.
+ */
+ public static String validate(String path) {
+ Path p = new Path(path);
+ if (p.toUri().getScheme() == null) {
+ throw new IllegalArgumentException("Invalid path provided, no scheme in " + path);
+ }
+ return path;
Review comment:
Did we want to have this normalize the path also by returning the normalized version from `Path`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369572339
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata;
+
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Utility class for validation of metadata tablet files.
+ */
+public class TabletFileUtil {
+
+ /**
+ * Validate if string is a valid path. Return same string or throw exception if not valid. This
+ * was added to facilitate more use of TabletFile over String but this puts the validation in one
+ * location in the case where TabletFile can't be used.
+ */
+ public static String validate(String path) {
+ Path p = new Path(path);
+ if (p.toUri().getScheme() == null) {
+ throw new IllegalArgumentException("Invalid path provided, no scheme in " + path);
+ }
+ return path;
Review comment:
I think that would be better. You thinking return the ```Path``` object or ```p.toString()```?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369612854
##########
File path: server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -614,7 +615,8 @@ private void deleteTablets(MergeInfo info) throws AccumuloException {
for (Entry<Key,Value> entry : scanner) {
Key key = entry.getKey();
if (key.compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- datafiles.add(new FileRef(this.master.fs, key));
+ datafiles
+ .add(new FileRef(TabletFileUtil.validate(key.getColumnQualifierData().toString())));
Review comment:
With a method like the following, could possibly do `datafiles.add(new FileRef(TabletFile.from(key)));`. Not completely sure about this, its just something I thought of while looking at this code.
```java
class TableFile {
public static TableFile from(Key k){
switch(k.getColumnFamilyData().toString()) {
case SCAN:
case DATA:
return new TableFile(k.getColumnQualifierData().toString());
}
}
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369606997
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##########
@@ -3217,7 +3216,7 @@ public void recover(VolumeManager fs, KeyExtent extent, List<LogEntry> logEntrie
Collections.sort(sorted, (e1, e2) -> (int) (e1.timestamp - e2.timestamp));
for (LogEntry entry : sorted) {
Path recovery = null;
- Path finished = RecoveryPath.getRecoveryPath(fs.getFullPath(FileType.WAL, entry.filename));
+ Path finished = RecoveryPath.getRecoveryPath(new Path(entry.filename));
Review comment:
Should this be validated?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369614515
##########
File path: server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -614,7 +615,8 @@ private void deleteTablets(MergeInfo info) throws AccumuloException {
for (Entry<Key,Value> entry : scanner) {
Key key = entry.getKey();
if (key.compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- datafiles.add(new FileRef(this.master.fs, key));
+ datafiles
+ .add(new FileRef(TabletFileUtil.validate(key.getColumnQualifierData().toString())));
Review comment:
Actually, probably would not want to add that method to TabletFile. I think it would be best if TabletFile were just a POJO in order to cleanly separate serialization and try to coral serialization code to a single place.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369763823
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
It's not purely formatting. I would argue that the overloaded method would help centralize the logic related to validation by simplifying calling code, and that, in turn, makes it easier to swap out later (because working with concrete types rather than Strings helps you create replacement code that satisfies all your caller use cases). Also, avoiding the `toString()` calls and using the concrete types instead, can help find a lot of situations where code is being unnecessarily converted back and forth to/from String.
But, I also don't like overloaded methods, so if you can find a better way (or think it's better as-is), that's fine with me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369244786
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
This would be more readable if the validate call wrapped the right-hand-side of the assignment to `tabletFilePath` rather than being embedded into a constructor inside a parameter to the `put` method call.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369662348
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##########
@@ -3217,7 +3216,7 @@ public void recover(VolumeManager fs, KeyExtent extent, List<LogEntry> logEntrie
Collections.sort(sorted, (e1, e2) -> (int) (e1.timestamp - e2.timestamp));
for (LogEntry entry : sorted) {
Path recovery = null;
- Path finished = RecoveryPath.getRecoveryPath(fs.getFullPath(FileType.WAL, entry.filename));
+ Path finished = RecoveryPath.getRecoveryPath(new Path(entry.filename));
Review comment:
I am not sure what we could validate with the WAL filenames. But the filename is coming from LogEntry so I think we should do it when the LogEntry is created, not after the TabletServer gets the list of files.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369572339
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.metadata;
+
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Utility class for validation of metadata tablet files.
+ */
+public class TabletFileUtil {
+
+ /**
+ * Validate if string is a valid path. Return same string or throw exception if not valid. This
+ * was added to facilitate more use of TabletFile over String but this puts the validation in one
+ * location in the case where TabletFile can't be used.
+ */
+ public static String validate(String path) {
+ Path p = new Path(path);
+ if (p.toUri().getScheme() == null) {
+ throw new IllegalArgumentException("Invalid path provided, no scheme in " + path);
+ }
+ return path;
Review comment:
I think that would be good. You thinking return the ```Path``` object or ```p.toString()```?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369598047
##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -419,66 +407,6 @@ public Path matchingFileSystem(Path source, String[] options) {
return null;
}
- @Override
- public Path getFullPath(TableId tableId, String path) {
Review comment:
There is another method in this class named `getVolumeByPath` that has relative path handling code. Would that be something to address in this PR? Or should that be another PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369741787
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
I think you are pointing to an overall problem with a lot of the internal code. One of my goals with the new ```TabletFile``` (similar to how we are trying to use Ample as much as possible) was to try and replace as much overuse of String and FileRef as we can. We have a ```LogEntry``` which I think is good for WALs but we might need some more internal types for strings that aren't TabletFiles.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369756078
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -149,7 +150,8 @@ private static KeyExtent fixSplit(ServerContext context, TableId tableId, Text m
for (Entry<Key,Value> entry : scanner3) {
if (entry.getKey().compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
- origDatafileSizes.put(new FileRef(context.getVolumeManager(), entry.getKey()),
+ String tabletFilePath = entry.getKey().getColumnQualifierData().toString();
+ origDatafileSizes.put(new FileRef(TabletFileUtil.validate(tabletFilePath)),
Review comment:
Either way, I think this could get replaced with ```TabletFile``` or ```Ample``` eventually.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369252481
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
Also, there's a lot of calls to `toString()` on the path to validate. This is the only method that calls validate without calling `toString()` first. However, many of this method's own callers call `toString()` first on the parameter. I think some of this could be simplified if everybody stopped calling `toString()`, and we kept the more concrete type, and only call `toString()` inside the validate method, if necessary. It simplifies a lot of the calling code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369760807
##########
File path: server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
##########
@@ -319,9 +319,9 @@ public void delete(SortedMap<String,String> confirmedDeletes) throws TableNotFou
// of deleting something that should not be deleted. Must not change value of delete
// variable because thats whats stored in metadata table.
log.debug("Volume replaced {} -> {}", delete, switchedDelete);
- fullPath = fs.getFullPath(FileType.TABLE, switchedDelete);
+ fullPath = new Path(switchedDelete);
Review comment:
These are delete markers that have no references and are therefore safe to delete. The code after this deletes the files and then deletes the markers from the metadata table.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1479:
Clean up relative path code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r369747733
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
##########
@@ -192,10 +192,9 @@ private BatchWriter createWriter(TableId tableId) {
}
}
- public static Mutation createDeleteMutation(ServerContext context, TableId tableId,
- String pathToRemove) {
- Path path = context.getVolumeManager().getFullPath(tableId, pathToRemove);
- Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path.toString())));
+ public static Mutation createDeleteMutation(String pathToRemove) {
+ String path = TabletFileUtil.validate(pathToRemove);
+ Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
Review comment:
I'm happy to defer to future improvements, if what can be done here is limited.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1479: Clean up relative path code
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1479: Clean up relative path code
URL: https://github.com/apache/accumulo/pull/1479#discussion_r370813752
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##########
@@ -3217,7 +3216,7 @@ public void recover(VolumeManager fs, KeyExtent extent, List<LogEntry> logEntrie
Collections.sort(sorted, (e1, e2) -> (int) (e1.timestamp - e2.timestamp));
for (LogEntry entry : sorted) {
Path recovery = null;
- Path finished = RecoveryPath.getRecoveryPath(fs.getFullPath(FileType.WAL, entry.filename));
+ Path finished = RecoveryPath.getRecoveryPath(new Path(entry.filename));
Review comment:
I checked and ```LogEntry.fromKeyValue(key, val)``` does some checking. And this is what is called when the data is read from the metadata in ```TabletMetadata```
----------------------------------------------------------------
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
With regards,
Apache Git Services