Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============7842455558863326405=="
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>
Date: Sun, 16 Mar 2014 03:39:48 -0000
Message-ID: <20140316033948.30484.65765@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Josh Elser" <noreply@reviews.apache.org>
References: <20140313193233.12824.70858@reviews.apache.org>
In-Reply-To: <20140313193233.12824.70858@reviews.apache.org>
Reply-To: "Josh Elser" <josh.elser@gmail.com>

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


> On March 13, 2014, 7:32 p.m., kturner wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java, line 69
> > <https://reviews.apache.org/r/19142/diff/2/?file=517719#file517719line69>
> >
> >     Filesystem is an abstract class.  I do not think using it as a Key is good.  Do all implementation of FileSystem have good implementations of equals() and hashCode()?

Good point -- I can't imagine that all implementations would have unique hashCodes across one another. Maybe using the URI for the FileSystem is a better idea? (e.g. hdfs://nn:port, file://, s3://bucket) I'm guessing that the scheme should help a bit.


> On March 13, 2014, 7:32 p.m., kturner wrote:
> > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java, line 315
> > <https://reviews.apache.org/r/19142/diff/2/?file=517719#file517719line315>
> >
> >     Tablets can point to files on volumes that were configured in the past, but are no longer configured.  For this case the correct filesystem needs to be returned, not the default filesystem.  The volumes currently configured are what should be used to create new files, but code should not assume those are the only volumes in use.

Noted. It's easy enough to fix that, but, without a proper definition in instance.volumes for that FileSystem, I can't create a "correct" Volume object (because we don't know what the base path within that Volume should be). I think this is OK because, like you said, we'll only read from that Volume and not try to create new files on that Volume.

I'll write up a new test for this as well -- should be easy enough to replicate.


- Josh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19142/#review37092
-----------------------------------------------------------


On March 12, 2014, 8:51 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, 8:51 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
> 
>


--===============7842455558863326405==--