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 28312: ACCUMULO-3199 Internal refactor to add ClientContext
Date Fri, 21 Nov 2014 22:29:51 GMT

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


Overall, it looks good. It moves us in a good direction. Well done on this so far.


core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
<https://reviews.apache.org/r/28312/#comment104635>

    More to do here yet or can this comment be removed?



core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java
<https://reviews.apache.org/r/28312/#comment104636>

    Some javadoc would be much appreciated.



server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java
<https://reviews.apache.org/r/28312/#comment104728>

    Why the removal of getting the namespace ID here and moving it into the other methods?



server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
<https://reviews.apache.org/r/28312/#comment104732>

    I'm torn on this -- I like not having an extra variable on TabletServer, but I'm cringing
a little bit at "leaking" TabletServer.
    
    I can't decide if it's detrimental or I just don't like the way it looks.



server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
<https://reviews.apache.org/r/28312/#comment104733>

    Same thing as over in TabletServer. I know you're just passing in the TabletServer instance
for the configuration, but it looks strange in this call.


- Josh Elser


On Nov. 21, 2014, 4:09 a.m., Christopher Tubbs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2014, 4:09 a.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/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