accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 30226: ACCUMULO-3204 Unused code
Date Fri, 30 Jan 2015 18:51:39 GMT


> On Jan. 30, 2015, 6: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?

It's what lets us run ITs against a "real" cluster.


> On Jan. 30, 2015, 6: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.

Ok, I mostly brought it up because I don't think we have an agreed upon precedent here.


> On Jan. 30, 2015, 6: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.

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.


> On Jan. 30, 2015, 6: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?

Testing


> On Jan. 30, 2015, 6: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?

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.


> On Jan. 30, 2015, 6: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.
> 
> Christopher Tubbs wrote:
>     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.

I think it's a sign that we have more data we could display that we're not in this specific
case.


> On Jan. 30, 2015, 6: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.

Again, a sign that test coverage could/should be improved


- Josh


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


On Jan. 23, 2015, 9: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, 9: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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message