> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, > > lines 57-64 > > <https://reviews.apache.org/r/28312/diff/1/?file=771856#file771856line57> > > > > Is this class meant to be thread-safe? These two methods are > > synchronized, but the rest of them aren't.
This method is the only place where we allow setting a member field after constructing, and the context is passed around a lot, including in internal threads, so yes, I want it to be thread-safe. However, it may not really matter, since the only place this is actually used is in the shell and SecurityOperationsImpl, and we only place the serialized credentials in the RPC thread pools. Much of what it replaces was already synchronized (synchronized access to HdfsZooInstance.getInstance() singleton, for example), so synchronization here isn't any different from what we had before. I think we can defer this to future analysis, unless a compelling reason can be made now to go one way or the other. > On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, > > lines 70-72 > > <https://reviews.apache.org/r/28312/diff/1/?file=771856#file771856line70> > > > > This method seems very specific. Maybe instead it should be a generic > > get(Property) method that delegates to the conf? I considered inline-ing this, but the primary purpose of the context object was to carry RPC-related config, and this is called a lot, so it's a nice convenience method. It's not public API, and it is rare to ask for any other config from the context, so I think it's fine for now, and can be deferred to future RPC abstraction improvements. > On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, > > lines 86-91 > > <https://reviews.apache.org/r/28312/diff/1/?file=771856#file771856line86> > > > > We could make this a utility method somewhere else. I'd really rather > > not see something "for internal use only" on the new face of our public API. This isn't public API. Nothing in this patch changes public API. The entire class is internal use only. So, the javadoc is redundant. I can remove the javadoc when I add a javadoc for the whole class, but it doesn't really matter. > On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java, > > lines 19-24 > > <https://reviews.apache.org/r/28312/diff/1/?file=771858#file771858line19> > > > > Is this editor auto-reformating? Please try to leave this out, if > > possible. I did my best to minimize editor changes, but some things passed through, occasionally, when it was more convenient to let the editor organize imports. It's a worthwhile tradeoff. > On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java, > > lines 91-99 > > <https://reviews.apache.org/r/28312/diff/1/?file=771864#file771864line91> > > > > Are these doing the same thing? This looks like a big change. Yes, they were doing the same thing. Look at InstanceOperationsImpl. This was simply removing redundant code. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28312/#review62669 ----------------------------------------------------------- On Nov. 20, 2014, 11:09 p.m., Christopher Tubbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28312/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2014, 11:09 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/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 > >
