On Nov. 24, 2014, 10:45 a.m., Christopher Tubbs wrote: > > Got through page 4. In general, I think we need more developer oriented > > documentation about when you should be passing a context and when it is > > safe to use a raw instance/connector/configuration.
Context replaces the need to pass the others around individually, and helps provide clarification for its use, but it does not preclude you from passing others around, when needed. A good example is the ZK-related utility code... it doesn't need credentials or configuration... just instanceId. For any thrift-RPC, the full context is usually needed. It's an improvement over the existing, but by no means is a perfect solution that forces us to use these things in a particular way. My hope is that it will be a stepping stone for future improvements. For example... a big area of confusion when debugging this change was the AccumuloReplicaSystem, which runs in the TabletServer. It has a TabletServer context, but it also has a ClientContext to talk to a remote peer, constructed from the current server's configuration. What this means is that the AccumuloReplicaSystem is limited to using the current server's SSL parameters to talk to the peer, because there is no way to configure the remote 's SSL params (currently). That wasn't necessarily obvious from before this change. Now, it is clear that there are two distinct contexts. That clarity is innate, though, because the context creates more readable code (IMO). I'm not arguing against more developer oriented documentation... but I do think that some design decisions can be helpful by their nature, by the impact it has on the code, and I'd hate to strictly prescribe how these things should be used in future. In any case, I think this is a first step... I'm sure there's lots of opportunities for redesign, and better documentation for internals. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28312/#review62775 ----------------------------------------------------------- On Nov. 21, 2014, 9:21 p.m., Christopher Tubbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28312/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2014, 9:21 p.m.) > > > Review request for accumulo and Mike Drob. > > > Bugs: ACCUMULO-3199 and ACCUMULO-3252 > https://issues.apache.org/jira/browse/ACCUMULO-3199 > https://issues.apache.org/jira/browse/ACCUMULO-3252 > > > Repository: accumulo > > > Description > ------- > > This patch introduces a new ClientContext object that contains Credentials, > Instance, and Configuration provided from the client API. This new object is > passed around internally in place of the previous three objects. An > AccumuloServerContext is also introduced, which extends the ClientContext. > Together, these objects ensure the proper configuration, credentials, and > everything else needed to communicate with other system components are > available to any RPC-related code. > > These new object types also reduce the need to create multiple references to > commonly used internal objects (such as HdfsZooInstance and > SystemCredentials), and avoids storing information in static fields. As a > side-effect, this should allow for better testing with mocked components. > > This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java > 4e11a14 > > core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java > bd76a50 > > core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java > 603172f > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java > cb0b90e > core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java > 2d077d9 > > core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java > 9848612 > core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java > 53f178a > > core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java > b09582a > > core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java > f756ef1 > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java > 901128b > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java > e45bcc9 > > core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java > 97d476b > core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java > 670782e > > core/src/main/java/org/apache/accumulo/core/client/impl/ScannerIterator.java > b0bfc20 > > core/src/main/java/org/apache/accumulo/core/client/impl/SecurityOperationsImpl.java > 8a17a77 > core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java > 2d76695 > > core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java > 3c450c3 > core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocator.java > 6c57c29 > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java > c550f15 > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchDeleter.java > 4858c7b > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java > c6f3a70 > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java > 0cb9f7f > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java > a73fdec > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java > 8e601d2 > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java > 3791d63 > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java > 4f3c661 > > core/src/main/java/org/apache/accumulo/core/client/impl/TimeoutTabletLocator.java > bcbe561 > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java 7b307d8 > > core/src/main/java/org/apache/accumulo/core/client/mock/impl/MockTabletLocator.java > 35f160f > > core/src/main/java/org/apache/accumulo/core/metadata/MetadataLocationObtainer.java > c7fe137 > core/src/main/java/org/apache/accumulo/core/metadata/MetadataServicer.java > 7a503b7 > > core/src/main/java/org/apache/accumulo/core/metadata/ServicerForMetadataTable.java > 0802994 > > core/src/main/java/org/apache/accumulo/core/metadata/ServicerForRootTable.java > 4da517c > > core/src/main/java/org/apache/accumulo/core/metadata/ServicerForUserTables.java > fd64e05 > > core/src/main/java/org/apache/accumulo/core/metadata/TableMetadataServicer.java > a3800ed > > core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java > 620381d > core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 88f5afc > > core/src/test/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelperTest.java > 663ce22 > > core/src/test/java/org/apache/accumulo/core/client/impl/ClientContextTest.java > PRE-CREATION > > core/src/test/java/org/apache/accumulo/core/client/impl/RootTabletLocatorTest.java > b7be982 > > core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java > 311bbf8 > > core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsImplTest.java > 20e068b > > core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java > 21bf2b9 > > core/src/test/java/org/apache/accumulo/core/metadata/MetadataServicerTest.java > 687e543 > > core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java > b404d3d > > mapreduce/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java > 5af78d2 > > mapreduce/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java > 836cff9 > > mapreduce/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/impl/InputConfigurator.java > 90a8622 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java > 5149d9d > > minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java > 2031b11 > > server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java > a2ff172 > > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java > 1b54ebd > > server/base/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java > efbd882 > > server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java > 7e36b40 > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java > 8cf46c7 > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java > baad742 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > a15e05e > > server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java > 3388b5b > > server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java > d96f9b0 > > server/base/src/main/java/org/apache/accumulo/server/master/balancer/TabletBalancer.java > 30a2460 > > server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java > 63617d6 > > server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataTableScanner.java > 155a5d2 > > server/base/src/main/java/org/apache/accumulo/server/master/state/RootTabletStateStore.java > 219fd49 > > server/base/src/main/java/org/apache/accumulo/server/master/state/TabletStateStore.java > 7c75454 > > server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReport.java > 99349b1 > > server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReportingIterator.java > 69d73e1 > > server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java > 23d4de5 > > server/base/src/main/java/org/apache/accumulo/server/replication/ReplicationUtil.java > 50b15f5 > > server/base/src/main/java/org/apache/accumulo/server/security/AuditedSecurityOperation.java > 8049003 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java > 71dcdcf > > server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java > 33a5158 > server/base/src/main/java/org/apache/accumulo/server/util/Admin.java > 254b62c > > server/base/src/main/java/org/apache/accumulo/server/util/FindOfflineTablets.java > 721d4e2 > > server/base/src/main/java/org/apache/accumulo/server/util/ListVolumesUsed.java > b876392 > > server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java > 01eb477 > > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java > 97b8cff > > server/base/src/main/java/org/apache/accumulo/server/util/RandomizeVolumes.java > 002659d > > server/base/src/main/java/org/apache/accumulo/server/util/RemoveEntriesForMissingFiles.java > 0b4f896 > > server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java > 8f0656c > server/base/src/main/java/org/apache/accumulo/server/util/TServerUtils.java > 470394d > > server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java > 50d7014 > > server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java > 3680341 > > server/base/src/test/java/org/apache/accumulo/server/conf/TableConfigurationTest.java > e66ec98 > > server/base/src/test/java/org/apache/accumulo/server/master/balancer/TableLoadBalancerTest.java > 5c134a5 > > server/base/src/test/java/org/apache/accumulo/server/problems/ProblemReportingIteratorTest.java > 6c61469 > > server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java > c8610d5 > > server/base/src/test/java/org/apache/accumulo/server/util/ReplicationTableUtilTest.java > 88f13c9 > > server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java > 4a9dc3e > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > f6ea8f7 > > server/gc/src/main/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferences.java > b0fd0f4 > > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java > 71e5f7d > > server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java > 6195f42 > > server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java > 03a2678 > server/master/src/main/java/org/apache/accumulo/master/Master.java 93691fb > > server/master/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java > 9a3e532 > > server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java > f5ed941 > > server/master/src/main/java/org/apache/accumulo/master/metrics/ReplicationMetrics.java > 762b18d > > server/master/src/main/java/org/apache/accumulo/master/replication/MasterReplicationCoordinator.java > 8218c8a > > server/master/src/main/java/org/apache/accumulo/master/replication/ReplicationDriver.java > f822a90 > > server/master/src/main/java/org/apache/accumulo/master/replication/WorkDriver.java > 975d06c > > server/master/src/main/java/org/apache/accumulo/master/replication/WorkMaker.java > e2ac08b > > server/master/src/main/java/org/apache/accumulo/master/tableOps/BulkImport.java > 66c3fcc > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CancelCompactions.java > 374fc24 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ChangeTableState.java > f1878b0 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneTable.java > da0afd8 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java > 13ef68e > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateNamespace.java > 8d0aa26 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateTable.java > 1e4d40e > > server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteNamespace.java > b6a9578 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteTable.java > 6a49a05 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ExportTable.java > 9c397db > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ImportTable.java > 35067ce > > server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTable.java > 5d511ac > > server/master/src/main/java/org/apache/accumulo/master/tableOps/TableRangeOp.java > 12849b6 > server/master/src/test/java/org/apache/accumulo/master/TestMergeState.java > 29dfefb > > server/master/src/test/java/org/apache/accumulo/master/replication/MasterReplicationCoordinatorTest.java > 1ec3f24 > > server/master/src/test/java/org/apache/accumulo/master/replication/WorkMakerTest.java > 0455c44 > > server/monitor/src/main/java/org/apache/accumulo/monitor/EmbeddedWebServer.java > 46fe54b > server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java > 8bc255d > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java > 2feb804 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/LogServlet.java > f877664 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/MasterServlet.java > 1b613e5 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java > c6c75cd > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ProblemServlet.java > 60cece6 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java > 94765f8 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/TServersServlet.java > 4409dff > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/TablesServlet.java > 428880e > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/VisServlet.java > fb56573 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/XMLServlet.java > 405d6df > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java > 53cb172 > server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java > fb14ca5 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 93161ee > > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java > ba86522 > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java > 91ec141 > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java > 71643e8 > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/BatchWriterReplicationReplayer.java > 358857d > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/ReplicationServicerHandler.java > 30361b1 > > server/tserver/src/main/java/org/apache/accumulo/tserver/replication/ReplicationWorker.java > 20da0d6 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java > 4f19ff9 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java > 3eb7229 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/MinorCompactor.java > 115aed7 > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java > 1f3306e > > server/tserver/src/test/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManagerTest.java > 26ec264 > > server/tserver/src/test/java/org/apache/accumulo/tserver/replication/BatchWriterReplicationReplayerTest.java > fee5dcd > shell/src/main/java/org/apache/accumulo/shell/Shell.java 996b2af > test/src/main/java/org/apache/accumulo/test/GetMasterStats.java 00f8f74 > test/src/main/java/org/apache/accumulo/test/WrongTabletTest.java 9089b60 > > test/src/main/java/org/apache/accumulo/test/continuous/ContinuousStatsCollector.java > 6ead9a6 > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > 592e177 > > test/src/main/java/org/apache/accumulo/test/performance/metadata/MetadataBatchScanTest.java > 96eb214 > > test/src/main/java/org/apache/accumulo/test/performance/scan/CollectTabletStats.java > bf21818 > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 438b83e > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Shutdown.java > 69d54a9 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/StartAll.java > 6b8c43c > > test/src/main/java/org/apache/accumulo/test/randomwalk/security/WalkingSecurity.java > 8a66951 > > test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java > 7524943 > > test/src/test/java/org/apache/accumulo/test/MasterRepairsDualAssignmentIT.java > da2d20f > test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java > 5dc75d0 > test/src/test/java/org/apache/accumulo/test/MultiTableBatchWriterIT.java > 484c048 > test/src/test/java/org/apache/accumulo/test/TableConfigurationUpdateIT.java > 0d9a211 > test/src/test/java/org/apache/accumulo/test/TotalQueuedIT.java a794088 > test/src/test/java/org/apache/accumulo/test/functional/AbstractMacIT.java > 644b05e > > test/src/test/java/org/apache/accumulo/test/functional/BalanceAfterCommsFailureIT.java > c32375b > > test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java > d828675 > > test/src/test/java/org/apache/accumulo/test/functional/DynamicThreadPoolsIT.java > 3ec3ccf > > test/src/test/java/org/apache/accumulo/test/functional/MasterAssignmentIT.java > 46f6b23 > > test/src/test/java/org/apache/accumulo/test/functional/MetadataMaxFiles.java > 98adbf6 > > test/src/test/java/org/apache/accumulo/test/functional/SimpleBalancerFairnessIT.java > 33dfa15 > test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java > 3f04b94 > test/src/test/java/org/apache/accumulo/test/functional/SplitRecoveryIT.java > 5223439 > > test/src/test/java/org/apache/accumulo/test/replication/GarbageCollectorCommunicatesWithTServersIT.java > 1044677 > > test/src/test/java/org/apache/accumulo/test/replication/UnorderedWorkAssignerReplicationIT.java > ad9c9fc > > Diff: https://reviews.apache.org/r/28312/diff/ > > > Testing > ------- > > Full ITs passed (after re-running failures manually). Also, ran > ReplicationITs with SSL enabled. > > > Thanks, > > Christopher Tubbs > >
