Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============6019633121083020171=="
MIME-Version: 1.0
Subject: Re: Review Request 19142: Deprecate instance.dfs.dir and
 instance.dfs.uri
From: "Josh Elser" <josh.elser@gmail.com>
To: "Eric Newton" <eric.newton@gmail.com>, keith@deenlo.com
Cc: "accumulo" <dev@accumulo.apache.org>, "Josh Elser" <josh.elser@gmail.com>,
 "Sean Busbey" <sean@manvsbeard.com>
Date: Thu, 20 Mar 2014 21:39:17 -0000
Message-ID: <20140320213917.24726.1312@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Josh Elser" <noreply@reviews.apache.org>
References: <20140320165555.6335.24496@reviews.apache.org>
In-Reply-To: <20140320165555.6335.24496@reviews.apache.org>
Reply-To: "Josh Elser" <josh.elser@gmail.com>

--===============6019633121083020171==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> 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
> 
>


--===============6019633121083020171==--