> On March 20, 2014, 4:55 p.m., Sean Busbey wrote: > > server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfiguration.java, > > lines 66-75 > > <https://reviews.apache.org/r/19142/diff/3/?file=520527#file520527line66> > > > > This "just pick one instance id" code is already repeated. probably > > worth pulling it to a common location. > > > > Maybe into the Accumulo class similar to the > > "getAccumuloPersistedVersion" call? Or as a method on VolumeManager? > > Josh Elser wrote: > I considered doing this, but opted against it for a few reasons. Most > importantly, if we're moving from the concept of a single Volume (or > FileSystem) to multiple, it doesn't make sense to allow a dev to write with > only one in mind. The cases where we do this are specific to the task at hand > being agnostic of multiple Volumes (in this case, assumption of instance_id > being uniform). For choosing which Volume a file should go on, there's > already the VolumeChooser and (RandomVolumeChooser impl). Unless you can > think of a good reason for this, I'd prefer to not encapsulate this to avoid > encouraging bad practice. > > Sean Busbey wrote: > I see instance id and persisted version as two special cases that are key > to the internals of a running Accumulo. I don't see why we rely a > consolidated Accumulo.getAccumuloPersistentVersion and yet don't do the same > with an Accumulo.getAccumuloInstanceId. > > Josh Elser wrote: > Pointing out that Accumulo#getAccumuloPersistentVersion(VolumeManager) is > one of the places that does this. I suppose it wouldn't be a bad idea to > follow suite and make a method in Accumulo that just takes a VolumeManager > and returns the instanceId rather than requiring caller to go through > ServerConstants and provide a Volume.
Just consolidated the two places this was done into Accumulo.java - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19142/#review37904 ----------------------------------------------------------- On March 16, 2014, 3:43 a.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19142/ > ----------------------------------------------------------- > > (Updated March 16, 2014, 3:43 a.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/NonConfiguredVolume.java > PRE-CREATION > 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 > > core/src/test/java/org/apache/accumulo/core/volume/NonConfiguredVolumeTest.java > PRE-CREATION > 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 > >
