> On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, > > line 184 > > <https://reviews.apache.org/r/30226/diff/1/?file=832019#file832019line184> > > > > I don't know what we want the policy to be here. No, this isn't used > > because we have no one-way thrift RPCs presently. This may change in the > > future.
If we don't need it now, and we don't know of any specific work that will leverage it shortly, I think we should strip it out, otherwise it risks getting stale. It may be good to save off the diff in a separate branch to add later as part of a larger patch to implement a feature that would leverage these. > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java, > > line 34 > > <https://reviews.apache.org/r/30226/diff/1/?file=832043#file832043line34> > > > > This is stubbed out to eventually support bulk imports Same reply as above. > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > examples/simple/src/main/java/org/apache/accumulo/examples/simple/filedata/ChunkInputStream.java, > > line 53 > > <https://reviews.apache.org/r/30226/diff/1/?file=832053#file832053line53> > > > > Example code could have been used by users. We might not want to remove > > things here? Examples are like documentation, in my opinion, and should be subject to change, because by its very nature as an example, we recognize that it's not worthy of being used directly. I also think it should be succinct. Too much code in the examples complicates them and makes them harder to teach the underlying principles. That's why I think unused code should probably be stripped out there. > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java, > > line 747 > > <https://reviews.apache.org/r/30226/diff/1/?file=832062#file832062line747> > > > > Should preserve the accompanying getter for the setter. Why is a setter needed if the getter is never used? > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java, > > line 49 > > <https://reviews.apache.org/r/30226/diff/1/?file=832061#file832061line49> > > > > I don't see the need to remove this. Would be happy to add a test so > > it's "used". I don't recall this one specifically. What's it for? > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > server/master/src/main/java/org/apache/accumulo/master/replication/DistributedWorkQueueWorkAssigner.java, > > line 71 > > <https://reviews.apache.org/r/30226/diff/1/?file=832094#file832094line71> > > > > I would prefer to not nuke these. It's just going to be more work when > > I inevitably find bugs and need to write more tests. Do you have any existing tests that could benefit from using it now? And, which is the greater risk? That there may be some extra effort needed later, or that the current unused method will become stale or broken until that time? > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java, line > > 144 > > <https://reviews.apache.org/r/30226/diff/1/?file=832098#file832098line144> > > > > Does this really mean that we're just not displaying it on the monitor? > > I think we'd want to do that rather than just throw away these stats. Yeah, so that's the kind of responses I was hoping for in this review... I don't know. Is this something that should be displayed, or is it really unneeded? Is it a bug? I just don't know enough about what this particular code was supposed to be doing, without taking a longer amount of time to investigate each instance of this kind of thing. > On Jan. 30, 2015, 1:21 p.m., Josh Elser wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java, > > line 88 > > <https://reviews.apache.org/r/30226/diff/1/?file=832130#file832130line88> > > > > Again, unnecessary removal of getters/setters IMO It seems that if we don't use the getter, we don't need the setter either. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30226/#review70370 ----------------------------------------------------------- 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 > >
