> On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/conf/Property.java, line 106 > > <https://reviews.apache.org/r/19142/diff/1/?file=517593#file517593line106> > > > > Do we need to describe what this functionality has been replaced with?
I'll add a subtask to ensure this gets covered in release notes (if not its own document) > On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/conf/Property.java, line 115 > > <https://reviews.apache.org/r/19142/diff/1/?file=517593#file517593line115> > > > > Do we need to describe what this functionality has been replaced with? I'll add a subtask to ensure this gets covered in release notes (if not its own document) > On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/volume/VolumeConfiguration.java, > > lines 37-38 > > <https://reviews.apache.org/r/19142/diff/1/?file=517599#file517599line37> > > > > Are we sure that ':' means absolute path? Couldn't a filename > > theoretically have a ':' in it? Likely need to be more defensive about this. The code is already riddled with these. If you have a volume with a colon in it, things will break wildly. There should probably be a ticket made to ensure that volumes (and replacements) do not contain them in the path of the URI > On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/volume/VolumeConfiguration.java, > > lines 47-59 > > <https://reviews.apache.org/r/19142/diff/1/?file=517599#file517599line47> > > > > This doesn't make sense. First we use our own depracated property, then > > we fall back on a hadoop default, and then on a deprecated hadoop default. > > Shouldn't we start with something non-deprecated? We aren't falling back onto a deprecated hadoop default -- we're using the Hadoop API which happens to still check a property they have already deprecated. We are still using own our deprecated property because it doesn't make sense to create a brand new property to support a feature that will be moot when we no longer support relative paths (assuming 1.7). This goes away when we no longer support relative paths. > On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java, > > line 354 > > <https://reviews.apache.org/r/19142/diff/1/?file=517608#file517608line354> > > > > Is reference equality still correct here? It's the same exact conditional that was there previously, so I have no reason as to why it wouldn't be. > On March 12, 2014, 7:45 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/volume/VolumeConfiguration.java, > > line 123 > > <https://reviews.apache.org/r/19142/diff/1/?file=517599#file517599line123> > > > > Check for bases[i] ends with "/" This wasn't a modification by me (present in the original VolumeConfiguration in a different package), but I added the check anyways. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19142/#review36958 ----------------------------------------------------------- On March 12, 2014, 6:57 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19142/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 6:57 p.m.) > > > Review request for accumulo, Eric Newton and kturner. > > > Bugs: ACCUMULO-2061 > https://issues.apache.org/jira/browse/ACCUMULO-2061 > > > Repository: accumulo > > > Description > ------- > > Removes instance.dfs.dir from being appended to the configured > instance.volumes values. Removed FileSystem from the VolumeManager in favor > of a new Volume class. > > The Volume class contains a FileSystem but also a base path which is denotes > what the Accumulo directory is on that FileSystem. > > > Diffs > ----- > > > core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java > 0b2f10e > core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java > c90d380 > core/src/main/java/org/apache/accumulo/core/conf/Property.java fc4d012 > core/src/main/java/org/apache/accumulo/core/file/VolumeConfiguration.java > fb8c6c8 > core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java > 4cfefad > > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/PrintInfo.java > f21190e > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java 0c2cf3c > core/src/main/java/org/apache/accumulo/core/volume/Volume.java PRE-CREATION > core/src/main/java/org/apache/accumulo/core/volume/VolumeConfiguration.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/zookeeper/ZooUtil.java de1b432 > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 2fa9051 > server/base/src/main/java/org/apache/accumulo/server/ServerConstants.java > 8983d08 > > server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java > dc9acf8 > > server/base/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java > 6993a0a > > server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfiguration.java > 32f6126 > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManager.java > f0c7083 > > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java > 80301ef > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java > da3baa6 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > 925f602 > > server/base/src/main/java/org/apache/accumulo/server/master/recovery/HadoopLogCloser.java > 7edc0cf > > server/base/src/main/java/org/apache/accumulo/server/master/recovery/MapRLogCloser.java > bba7ac5 > server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java > ac13034 > server/base/src/main/java/org/apache/accumulo/server/util/FileUtil.java > 8e38cbd > > server/base/src/main/java/org/apache/accumulo/server/util/LocalityCheck.java > a96e791 > > server/base/src/main/java/org/apache/accumulo/server/util/TabletOperations.java > b237cd0 > > server/base/src/main/java/org/apache/accumulo/server/util/ZooKeeperMain.java > 37edb1a > > server/base/src/test/java/org/apache/accumulo/server/ServerConstantsTest.java > a316155 > server/base/src/test/java/org/apache/accumulo/server/fs/FileTypeTest.java > 205a793 > server/base/src/test/java/org/apache/accumulo/server/fs/VolumeUtilTest.java > c85be45 > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java > 4e2a878 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ExportTable.java > 36bbb53 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ImportTable.java > 7e84c55 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/DefaultServlet.java > 942f866 > > server/tserver/src/main/java/org/apache/accumulo/tserver/BulkFailedCopyProcessor.java > e9f1083 > server/tserver/src/main/java/org/apache/accumulo/tserver/Compactor.java > 151db6e > server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java > bb95532 > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java > cc4b68d > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 475621b > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/MajorCompactionRequest.java > 3bbb476 > server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java > 8f783c3 > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java > a28bac4 > server/tserver/src/test/java/org/apache/accumulo/tserver/RootFilesTest.java > 1cd8f12 > > server/tserver/src/test/java/org/apache/accumulo/tserver/TabletServerSyncCheckTest.java > 50c8b31 > > server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java > c4d3dfb > > server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java > 359bfa1 > > server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java > af149fa > > test/src/main/java/org/apache/accumulo/test/performance/scan/CollectTabletStats.java > 9a9cad7 > test/src/test/java/org/apache/accumulo/test/VolumeIT.java a0efe45 > test/src/test/java/org/apache/accumulo/test/functional/BulkFileIT.java > c8023c0 > > Diff: https://reviews.apache.org/r/19142/diff/ > > > Testing > ------- > > All unit and integration tests run. Tested locally with multiple volumes on a > single HDFS. Tested upgrade from 1.5 with volumes (verified that relative > paths were rewritten as data was ingested). Tested volume replacement on a > new instance. All "real instance" tests used CI to generate data. > > > Thanks, > > Josh Elser > >
