accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext
Date Mon, 24 Nov 2014 20:38:55 GMT


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
> 
>


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