> 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. > > Christopher Tubbs wrote: > 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. > > Josh Elser wrote: > IMO, an interface and a stub method throwing an Exception likely isn't > going to turn stale. Mostly bringing up why it was there at all.
Maybe... but it also might not be useful at all in the eventual implementation. Or, with it there, somebody could use it for something other than what was intended. I think it might be better to just strip it out, keep those extra methods in a working branch for the eventual feature it is intended for, if necessary. Keith suggested we could just comment on the ticket about the commit in which it was removed, to save anybody who eventually implements it some effort. I don't really care that much. From my perspective, it's just another bit of unused code. It's not obvious it's for a planned future feature, or from an long-dead one. > 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? > > Christopher Tubbs wrote: > 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. > > Josh Elser wrote: > Ok, I mostly brought it up because I don't think we have an agreed upon > precedent here. You're right, of course. We don't. Are you leaning towards an opinion about this? (I'm leaning towards stripping it out.) > 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". > > Christopher Tubbs wrote: > I don't recall this one specifically. What's it for? > > Josh Elser wrote: > It's what lets us run ITs against a "real" cluster. Ah, we should definitely have a test for it, then. > 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. > > Christopher Tubbs wrote: > Why is a setter needed if the getter is never used? > > Josh Elser wrote: > Testing Hmm, I see. It is low-risk, but it is confusing that it's not used... because it seems like it should be if it's there. If this were API, I'd definitely be in favor of keeping symmetric getters/setters, but for testing on an impl class... I'm not heartbroken about stripping it out. > 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. > > Christopher Tubbs wrote: > 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? > > Josh Elser wrote: > Not having/using the getters/setters make me think that coverage on the > tests could be improved. I don't see getters/setters going stale, and their > hypothetical removal just causing more work later. At the very least, they'd be at no risk of removal by me if they had tests. I'm still not clear on their intended future purpose... and think that if that work is in progress (even slow progress), maybe these extra methods should live in a branch for that feature, and should be added when they are needed. > 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 > > Christopher Tubbs wrote: > It seems that if we don't use the getter, we don't need the setter either. > > Josh Elser wrote: > Again, a sign that test coverage could/should be improved Do any of these fall into what you consider API for replication? Not "public API", of course, but API in the sense that it's an essential function of the replication feature that could be leveraged to create an alternate implementation? If so, I think: needs tests. If not, I think: delete. - 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 > >
