> On April 8, 2014, 7:59 a.m., Eric Newton wrote: > > It's kind of a big structural change for 1.6.1. Can we keep it just to 1.7?
You're right, I probably bit off too much here. The ultimate goal is improving testability, which I'd kinda like in 1.6 as well. How about I reduce the changeset so that most of the changed code goes back to the way it was, and the server ZooReaderWriter class remains, and only the factory and minimal updates to support it are added? Then the "future changes" can still come, but more slowly and in isolated pieces. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20101/#review39782 ----------------------------------------------------------- On April 7, 2014, 3:39 p.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20101/ > ----------------------------------------------------------- > > (Updated April 7, 2014, 3:39 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2212 > https://issues.apache.org/jira/browse/ACCUMULO-2212 > > > Repository: accumulo > > > Description > ------- > > The o.a.a.server.zookeeper.ZooReaderWriter class is converted to a factory > class, and all users of it are switched over. Classes that used > server.zookeeper.ZooReaderWriter objects now directly use the Fate class > instead, and the former class is eliminated. The logic that understands how > to construct ZooReaderWriters based on the site configuration resides now in > the new ZooReaderWriterFactory. > > Additional changes: > * Some method arguments are changed from being of type > o.a.a.fate.zookeeper.ZooReader to o.a.a.fate.zookeeper.IZooReader. > * The getZooKeeper() method of ZooReader was added to IZooReader, so that > ZooCache could work with the interface type. > * The invocation handler used to retry ZK calls on connection loss is > refactored into a new RetryingInvocationHandler. > > Notes for reviewers: > * The Fate classes should not depend on any other Accumulo packages. > * The goal is to facilitate future changes to the modified classes that add > setters for the factory used, instead of always using 'new > ZooReaderWriterFactory()'. The factory can then be made a mock for testing, > or dynamically changed to some alternative implementation at runtime. This > ticket is focused on moving the code to the factory and removing > o.a.a.server.zookeeper.ZooReaderWriter. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/trace/DistributedTrace.java > 83f5c26 > core/src/main/java/org/apache/accumulo/core/trace/ZooTraceClient.java > 1315a9d > fate/src/main/java/org/apache/accumulo/fate/zookeeper/IZooReader.java > 0610e79 > > fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryingInvocationHandler.java > PRE-CREATION > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java e793a69 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java > 60660d6 > fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java > 2a327b0 > > fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryingInvocationHandlerTest.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 4e1eb35 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > 5cbffc3 > > server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java > 63bd894 > > server/base/src/main/java/org/apache/accumulo/server/master/state/DeadServerList.java > 2f657c4 > > server/base/src/main/java/org/apache/accumulo/server/master/state/ZooStore.java > b0ed03f > > server/base/src/main/java/org/apache/accumulo/server/monitor/LogService.java > 0a5341a > > server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReport.java > b882195 > > server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java > 23d4de5 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java > 1646a28 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java > 34d43f2 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java > 6319653 > > server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java > 7a61eb6 > > server/base/src/main/java/org/apache/accumulo/server/tablets/UniqueNameAllocator.java > 4ae8335 > server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java > 2926a3f > > server/base/src/main/java/org/apache/accumulo/server/util/CleanZookeeper.java > f2074a1 > > server/base/src/main/java/org/apache/accumulo/server/util/DeleteZooInstance.java > 448da86 > > server/base/src/main/java/org/apache/accumulo/server/util/DumpZookeeper.java > 30aa2eb > > server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java > e936b97 > > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java > 374017d > > server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java > 4e5df9e > > server/base/src/main/java/org/apache/accumulo/server/util/RestoreZookeeper.java > 6e5607e > > server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java > b6ca527 > > server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java > bcaf9b0 > > server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java > 2fc0bd3 > server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java > 489d4bc > > server/base/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java > ac3426e > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java > c5a9528 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/TransactionWatcher.java > 4e0e977 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooCache.java > bf34ef6 > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooLock.java > dce6d38 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooQueueLock.java > f7c1e68 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriter.java > f950077 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriterFactory.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/problems/ProblemReportTest.java > dbad326 > > server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java > ab2ab42 > server/master/src/main/java/org/apache/accumulo/master/Master.java 2440ee4 > > server/master/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java > 8023169 > > server/master/src/main/java/org/apache/accumulo/master/state/MergeStats.java > 4737b6e > > server/master/src/main/java/org/apache/accumulo/master/state/SetGoalState.java > f981bae > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CancelCompactions.java > 49227ef > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java > e3b0405 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameNamespace.java > 41f24cd > > server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTable.java > 8c5ed00 > server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java > 577f5d5 > > server/master/src/main/java/org/apache/accumulo/master/tserverOps/ShutdownTServer.java > 20b7328 > server/master/src/main/java/org/apache/accumulo/master/util/FateAdmin.java > 4e72832 > server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java > 0e6b37f > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 67f2739 > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java > 3fe60b7 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 6d73125 > test/src/main/java/org/apache/accumulo/test/functional/CacheTestClean.java > 3fe94e1 > test/src/main/java/org/apache/accumulo/test/functional/CacheTestWriter.java > 20ea55f > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > f26c8d7 > > test/src/test/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java > f04f196 > test/src/test/java/org/apache/accumulo/test/functional/SplitRecoveryIT.java > d9de5d1 > > Diff: https://reviews.apache.org/r/20101/diff/ > > > Testing > ------- > > Unit tests pass; functional tests pass. > > > Thanks, > > Bill Havanki > >
