> 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. > > Christopher Tubbs wrote: > 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. > > Mike Drob wrote: > If we're going to defer things to the future, I'd feel more comfortable > with a TODO and an open JIRA to make sure it doesn't get lost. > > Christopher Tubbs wrote: > I can do that.
Upon further examination, and consulting with Keith Turner, I've convinced myself this synchronization is necessary. The user can change their credentials with the SecurityOperations API, and there could be other threads in the client that are using the old credentials. This can result in an inconsistent view of the current user's credentials and could cause some RPC calls to succeed while others fail, for extended periods of time. So, yes, the synchronization is necessary. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28312/#review62669 ----------------------------------------------------------- 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 > >
