Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 6900F109C6 for ; Thu, 29 Jan 2015 03:55:49 +0000 (UTC) Received: (qmail 29894 invoked by uid 500); 29 Jan 2015 03:55:49 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 29855 invoked by uid 500); 29 Jan 2015 03:55:49 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 29836 invoked by uid 99); 29 Jan 2015 03:55:49 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 29 Jan 2015 03:55:49 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id A41661D5C7C; Thu, 29 Jan 2015 03:55:44 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============6993030597886955433==" MIME-Version: 1.0 Subject: Re: Review Request 30226: ACCUMULO-3204 Unused code From: "Christopher Tubbs" To: "Christopher Tubbs" , "accumulo" , keith@deenlo.com Date: Thu, 29 Jan 2015 03:55:44 -0000 Message-ID: <20150129035544.25679.46410@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Christopher Tubbs" X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/30226/ X-Sender: "Christopher Tubbs" References: <20150128180754.25680.51603@reviews.apache.org> In-Reply-To: <20150128180754.25680.51603@reviews.apache.org> Reply-To: "Christopher Tubbs" X-ReviewRequest-Repository: accumulo --===============6993030597886955433== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Jan. 28, 2015, 1:07 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/iterators/DebugIterator.java, line 47 > > > > > > I know its not officially public API, but we do encourage users to use Iterators. This is something a user could have possible used. What would they use the constructor for? They might leverage iterators, but I can't imagine they'd construct them. The framework should do that. > On Jan. 28, 2015, 1:07 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/file/rfile/SplitLarge.java, line 82 > > > > > > this is a nice catch FWIW, all these were fixed in separate tickets, too, but I fixed them after I did this pass. > On Jan. 28, 2015, 1:07 p.m., kturner wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/ConditionalMutationSet.java, line 40 > > > > > > In addition to finding unused code, I suppose the tool helps you find code with a higher than needed visibility? Yes. You could say "unused at the particular visibility level". I started to track some of those, but they were too numerous, so I stopped. > On Jan. 28, 2015, 1:07 p.m., kturner wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java, line 511 > > > > > > So the tool let you know this code was only used by test? Yes. I believe this was caught by the higher-than-necessary visibility checks that I abandoned. I also replaced some comments with this annotation on other methods. I don't plan on committing those as part of this overall issue, though. If I do those changes, I'll do them separately. > On Jan. 28, 2015, 1:07 p.m., kturner wrote: > > server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java, line 166 > > > > > > Is this thrift method used? Will have to check. If this was the only method using that thrift method, then perhaps not. Should it be? - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30226/#review70025 ----------------------------------------------------------- On Jan. 23, 2015, 4:02 p.m., Christopher Tubbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30226/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2015, 4:02 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3204 > https://issues.apache.org/jira/browse/ACCUMULO-3204 > > > Repository: accumulo > > > Description > ------- > > I ran UCDetector in Eclipse and found about 2000 lines of code which could (maybe?) be removed. > > Some of this code might represent bugs (the code is supposed to be used, but isn't, because of a mistake... and I've already found a few of those cases), some of it is unused because it's loaded dynamically, via reflection, or it's required for a particular framework, or it's just stale code which is safe to remove. I'd like the community's help in determining which is which by giving some feedback on this rough first pass. > > UCDetector also detects code with a greater visibility than necessary. Some of this patch includes those... I started ignoring those after a bit and just focused on completely unused code, so you will see a few of those, but not as many as there could be. > > I tried not to include any public API in these changes, but I may have missed some. > > Some code might only be used in tests, too. I don't think I caught those here. Some code also is unused, but is public API, and should minimally have unit tests to verify public API functionality. I've tried to open JIRA issues for those, as I found them. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/bloomfilter/Filter.java 12961a9 > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java a449389 > core/src/main/java/org/apache/accumulo/core/client/impl/ScannerIterator.java 276e1d6 > core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 5b6d9ac > core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java 20b1639 > core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java f171889 > core/src/main/java/org/apache/accumulo/core/client/mock/MockConfiguration.java 8c57c5e > core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 2244d20 > core/src/main/java/org/apache/accumulo/core/conf/Property.java 68fac73 > core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java f0c9e59 > core/src/main/java/org/apache/accumulo/core/data/PartialKey.java f4289d2 > core/src/main/java/org/apache/accumulo/core/file/blockfile/ABlockReader.java 8df2469 > core/src/main/java/org/apache/accumulo/core/file/blockfile/ABlockWriter.java ece0a5e > core/src/main/java/org/apache/accumulo/core/file/blockfile/BlockFileWriter.java 3bdbea3 > core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java a6c08ff > core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/CachedBlock.java b6d6d41 > core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/ClassSize.java 2d7586f > core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/CachableBlockFile.java 4d65c9f > core/src/main/java/org/apache/accumulo/core/file/keyfunctor/ColumnFamilyFunctor.java 3660291 > core/src/main/java/org/apache/accumulo/core/file/rfile/SplitLarge.java b87705c > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java ecc0b90 > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/CompareUtils.java d7651e8 > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Utils.java 84b861b > core/src/main/java/org/apache/accumulo/core/iterators/DebugIterator.java 92f49f9 > core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 534dd7f > core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java fccafc5 > core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java ed46130 > core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java bc04480 > core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java d880fb3 > core/src/main/java/org/apache/accumulo/core/util/AddressUtil.java 1426239 > core/src/main/java/org/apache/accumulo/core/util/ByteArraySet.java ca43469 > core/src/main/java/org/apache/accumulo/core/util/Daemon.java a2c9e79 > core/src/main/java/org/apache/accumulo/core/util/MapCounter.java f6f3ff7 > core/src/main/java/org/apache/accumulo/core/util/StopWatch.java ddb612f > core/src/main/java/org/apache/accumulo/core/volume/VolumeConfiguration.java 99032ad > examples/simple/src/main/java/org/apache/accumulo/examples/simple/filedata/ChunkInputStream.java c902271 > examples/simple/src/main/java/org/apache/accumulo/examples/simple/mapreduce/TeraSortIngest.java 9aac75e > fate/src/main/java/org/apache/accumulo/fate/util/Daemon.java da7c41c > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java 5cfdbb8 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java a0100b2 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooQueueLock.java f9195f3 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java 6b5ec43 > minicluster/src/main/java/org/apache/accumulo/cluster/RemoteShell.java 5a44acf > minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java 58536ed > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java 76f332b > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/ZooKeeperBindException.java 50217ce > server/base/src/main/java/org/apache/accumulo/server/ServerConstants.java 9d969d1 > server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 01d03ed > server/base/src/main/java/org/apache/accumulo/server/conf/NamespaceConfWatcher.java 945e904 > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfWatcher.java 3c8d45d > server/base/src/main/java/org/apache/accumulo/server/conf/TableParentConfiguration.java bd2e5ab > server/base/src/main/java/org/apache/accumulo/server/conf/ZooConfigurationFactory.java 4fbb645 > server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java c0580ac > server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java 917d8d2 > server/base/src/main/java/org/apache/accumulo/server/master/state/DistributedStoreException.java 3290075 > server/base/src/main/java/org/apache/accumulo/server/master/state/TServerInstance.java c0c71e6 > server/base/src/main/java/org/apache/accumulo/server/master/state/TabletServerState.java dde9807 > server/base/src/main/java/org/apache/accumulo/server/metrics/AbstractMetricsImpl.java 93db9c8 > server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java b0ffd64 > server/base/src/main/java/org/apache/accumulo/server/rpc/TBufferedServerSocket.java 2887f48 > server/base/src/main/java/org/apache/accumulo/server/rpc/TNonblockingServerSocket.java d035862 > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java 7adb46e > server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java 6014139 > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthorizor.java b047f1a > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosPermissionHandler.java 691c555 > server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java 7e5f54d > server/base/src/main/java/org/apache/accumulo/server/util/AccumuloStatus.java 1e75124 > server/base/src/main/java/org/apache/accumulo/server/util/CleanZookeeper.java 3cfb1a7 > server/base/src/main/java/org/apache/accumulo/server/util/FileSystemMonitor.java 94a80d9 > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ed7626e > server/base/src/main/java/org/apache/accumulo/server/util/RestoreZookeeper.java e5a2add > server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java 759d898 > server/base/src/main/java/org/apache/accumulo/server/util/time/RelativeTime.java bc48b10 > server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java 556e6b9 > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooCache.java aca9c82 > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java da0b07c > server/master/src/main/java/org/apache/accumulo/master/replication/DistributedWorkQueueWorkAssigner.java 3e966c4 > server/master/src/main/java/org/apache/accumulo/master/replication/SequentialWorkAssigner.java e30e9ac > server/master/src/main/java/org/apache/accumulo/master/replication/StatusMaker.java c7f47e4 > server/master/src/main/java/org/apache/accumulo/master/replication/WorkDriver.java 3558d2d > server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java 1a2904c > server/monitor/src/main/java/org/apache/accumulo/monitor/ZooKeeperStatus.java 89e879e > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceDump.java 64fee7e > server/tracer/src/main/java/org/apache/accumulo/tracer/ZooTraceClient.java 3db77f0 > server/tserver/src/main/java/org/apache/accumulo/tserver/ActiveAssignmentRunnable.java dcbdae7 > server/tserver/src/main/java/org/apache/accumulo/tserver/BulkFailedCopyProcessor.java cfb5fb4 > server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionQueue.java 0cb04a7 > server/tserver/src/main/java/org/apache/accumulo/tserver/ConditionalMutationSet.java 79a1176 > server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java 0a7de95 > server/tserver/src/main/java/org/apache/accumulo/tserver/HoldTimeoutException.java 1bd2c2c > server/tserver/src/main/java/org/apache/accumulo/tserver/InMemoryMap.java 47936b6 > server/tserver/src/main/java/org/apache/accumulo/tserver/MemValue.java 0ce3b9e > server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java c2e45c3 > server/tserver/src/main/java/org/apache/accumulo/tserver/RunnableStartedAt.java 6513091 > server/tserver/src/main/java/org/apache/accumulo/tserver/TLevel.java 5705c9e > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7d49e65 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 351d526 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletStatsKeeper.java 1e2cdf4 > server/tserver/src/main/java/org/apache/accumulo/tserver/TooManyFilesException.java 026f7e2 > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/MajorCompactionRequest.java 900600f > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/strategies/ConfigurableCompactionStrategy.java ba3ea42 > server/tserver/src/main/java/org/apache/accumulo/tserver/constraints/UnsatisfiableConstraint.java 64bc2cd > server/tserver/src/main/java/org/apache/accumulo/tserver/data/ServerConditionalMutation.java 84137cc > server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 5acf5eb > server/tserver/src/main/java/org/apache/accumulo/tserver/log/LocalWALRecovery.java 2658c1f > server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java 405ec70 > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java 5c3fc2d > server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogFileKey.java 829cf2f > server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogFileValue.java 9ca0f38 > server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java df2f831 > server/tserver/src/main/java/org/apache/accumulo/tserver/mastermessage/SplitReportMessage.java 0c93a86 > server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerMetricsUtil.java 5905aea > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java a07f354 > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/BatchWriterReplicationReplayer.java 8a80ea3 > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/ReplicationProcessor.java c23cd94 > server/tserver/src/main/java/org/apache/accumulo/tserver/session/Session.java 9aaa17a > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactionStats.java 68a2307 > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/KVEntry.java 4b1cf8c > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Rate.java a0ea2d6 > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java a73356d > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/TabletClosedException.java d3ed507 > shell/src/main/java/org/apache/accumulo/shell/Shell.java a64ff45 > shell/src/main/java/org/apache/accumulo/shell/ShellCommandException.java 49e7163 > shell/src/main/java/org/apache/accumulo/shell/ShellCompletor.java 3ed6a04 > shell/src/main/java/org/apache/accumulo/shell/ShellOptions.java 302a8a9 > shell/src/main/java/org/apache/accumulo/shell/Token.java cd25ada > shell/src/main/java/org/apache/accumulo/shell/commands/CompactCommand.java 131534f > shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java 5917b1e > start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java 1cee6d7 > > Diff: https://reviews.apache.org/r/30226/diff/ > > > Testing > ------- > > > Thanks, > > Christopher Tubbs > > --===============6993030597886955433==--