diff --git a/core/src/main/spotbugs/exclude-filter.xml b/core/src/main/spotbugs/exclude-filter.xml index f1776b30607..c503d6f344f 100644 --- a/core/src/main/spotbugs/exclude-filter.xml +++ b/core/src/main/spotbugs/exclude-filter.xml @@ -43,4 +43,14 @@ + + + + + + + + + + diff --git a/core/src/test/java/org/apache/accumulo/core/logging/EscalatingLoggerTest.java b/core/src/test/java/org/apache/accumulo/core/logging/EscalatingLoggerTest.java index a119601cf0a..33f340a45fb 100644 --- a/core/src/test/java/org/apache/accumulo/core/logging/EscalatingLoggerTest.java +++ b/core/src/test/java/org/apache/accumulo/core/logging/EscalatingLoggerTest.java @@ -48,8 +48,8 @@ public void test() throws InterruptedException { // Programatically modify the Log4j2 Logging configuration to add an appender LoggerContext ctx = LoggerContext.getContext(false); Configuration cfg = ctx.getConfiguration(); - PatternLayout layout = PatternLayout.newBuilder().withConfiguration(cfg) - .withPattern(PatternLayout.SIMPLE_CONVERSION_PATTERN).build(); + PatternLayout layout = PatternLayout.newBuilder().setConfiguration(cfg) + .setPattern(PatternLayout.SIMPLE_CONVERSION_PATTERN).build(); Appender appender = WriterAppender.createAppender(layout, null, writer, "EscalatingLoggerTestAppender", false, true); appender.start(); diff --git a/pom.xml b/pom.xml index 9df56e7e19b..470c5be634e 100644 --- a/pom.xml +++ b/pom.xml @@ -171,7 +171,7 @@ under the License. 5.8.0 2.41.0 3.3.6 - 2.25.4 + 2.26.1 1.48.0 2.0.9 2.0.17 @@ -277,7 +277,7 @@ under the License. com.github.spotbugs spotbugs-annotations - 4.9.3 + 4.10.3 com.google.auto.service @@ -780,7 +780,7 @@ under the License. com.github.spotbugs spotbugs-maven-plugin - 4.9.8.2 + 4.10.3.0 true Max diff --git a/server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java b/server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java index 61457d907e0..ce5d8ee7546 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java +++ b/server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java @@ -475,7 +475,7 @@ private Map> assignMapFiles(VolumeManager fs, } private class AssignmentTask implements Runnable { - final Map> assignmentFailures; + private final Map> assignmentFailures; HostAndPort location; private Map> assignmentsPerTablet; diff --git a/server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionWatcher.java b/server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionWatcher.java index 47e75fd42d4..a6134ed1a17 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionWatcher.java +++ b/server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionWatcher.java @@ -45,6 +45,7 @@ public static void setTimer(LongTaskTimer ltt) { private final AccumuloConfiguration config; private static boolean watching = false; private static LongTaskTimer timer = null; + private static final Object lock = new Object(); private static class ObservedCompactionInfo { final CompactionInfo compactionInfo; @@ -121,11 +122,14 @@ public void run() { } } - public static synchronized void startWatching(ServerContext context) { - if (!watching) { - ThreadPools.watchCriticalScheduledTask(context.getScheduledExecutor().scheduleWithFixedDelay( - new CompactionWatcher(context.getConfiguration()), 10000, 10000, TimeUnit.MILLISECONDS)); - watching = true; + public static void startWatching(ServerContext context) { + synchronized (lock) { + if (!watching) { + ThreadPools.watchCriticalScheduledTask(context.getScheduledExecutor() + .scheduleWithFixedDelay(new CompactionWatcher(context.getConfiguration()), 10000, 10000, + TimeUnit.MILLISECONDS)); + watching = true; + } } } diff --git a/server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java b/server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java index 4e81899ed2f..ecfde27baf2 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java +++ b/server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java @@ -111,6 +111,7 @@ public int hashCode() { private final long slowFilePermitMillis; private final ServerContext context; + private final Object lock = new Object(); private class IdleFileCloser implements Runnable { @@ -123,7 +124,7 @@ public void run() { // determine which files to close in a sync block, and then close the // files outside of the sync block - synchronized (FileManager.this) { + synchronized (lock) { Iterator>> iter = openFiles.entrySet().iterator(); while (iter.hasNext()) { Entry> entry = iter.next(); diff --git a/server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java b/server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java index c08d470d276..527454ecbfd 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java +++ b/server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java @@ -59,6 +59,7 @@ public class ProblemReports implements Iterable { private static final Logger log = LoggerFactory.getLogger(ProblemReports.class); private final LRUMap problemReports = new LRUMap<>(1000); + private static final Object lock = new Object(); /** * use a thread pool so that reporting a problem never blocks @@ -283,12 +284,14 @@ public Iterator iterator() { return iterator(null); } - public static synchronized ProblemReports getInstance(ServerContext context) { - if (instance == null) { - instance = new ProblemReports(context); - } + public static ProblemReports getInstance(ServerContext context) { + synchronized (lock) { + if (instance == null) { + instance = new ProblemReports(context); + } - return instance; + return instance; + } } public static void main(String[] args) { diff --git a/server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java b/server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java index 7c40d0ef56b..081d5b34dea 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java +++ b/server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java @@ -35,6 +35,7 @@ public class StatusUtil { private static final Value INF_END_REPLICATION_STATUS_VALUE, CLOSED_STATUS_VALUE; private static final Status.Builder CREATED_STATUS_BUILDER; + private static final Object lock = new Object(); static { CREATED_STATUS_BUILDER = Status.newBuilder(); @@ -119,11 +120,13 @@ public static Status replicatedAndIngested(Status.Builder builder, long recordsR /** * @return A {@link Status} for a new file that was just created */ - public static synchronized Status fileCreated(long timeCreated) { - // We're using a shared builder, so we need to synchronize access on it until we make a Status - // (which is then immutable) - CREATED_STATUS_BUILDER.setCreatedTime(timeCreated); - return CREATED_STATUS_BUILDER.build(); + public static Status fileCreated(long timeCreated) { + synchronized (lock) { + // We're using a shared builder, so we need to synchronize access on it until we make a Status + // (which is then immutable) + CREATED_STATUS_BUILDER.setCreatedTime(timeCreated); + return CREATED_STATUS_BUILDER.build(); + } } /** diff --git a/server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java b/server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java index f998f148839..7d82beb1259 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java +++ b/server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java @@ -56,6 +56,7 @@ public class KerberosAuthenticator implements Authenticator { private ServerContext context; private String zkUserPath; private UserImpersonation impersonation; + private final Object zooCacheLock = new Object(); @Override public void initialize(ServerContext context) { @@ -72,7 +73,7 @@ public boolean validSecurityHandlers() { } private void createUserNodeInZk(String principal) throws KeeperException, InterruptedException { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); ZooReaderWriter zoo = context.getZooReaderWriter(); zoo.putPrivatePersistentData(zkUserPath + "/" + principal, new byte[0], @@ -85,7 +86,7 @@ public void initializeSecurity(String principal, byte[] token) { try { // remove old settings from zookeeper first, if any ZooReaderWriter zoo = context.getZooReaderWriter(); - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); if (zoo.exists(zkUserPath)) { zoo.recursiveDelete(zkUserPath, NodeMissingPolicy.SKIP); diff --git a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java index bdd41135b31..e830917782a 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java +++ b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java @@ -48,6 +48,7 @@ public final class ZKAuthenticator implements Authenticator { private ServerContext context; private String ZKUserPath; private ZooCache zooCache; + private final Object zooCacheLock = new Object(); @Override public void initialize(ServerContext context) { @@ -88,7 +89,7 @@ public void initializeSecurity(String principal, byte[] token) { try { // remove old settings from zookeeper first, if any ZooReaderWriter zoo = context.getZooReaderWriter(); - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); if (zoo.exists(ZKUserPath)) { zoo.recursiveDelete(ZKUserPath, NodeMissingPolicy.SKIP); @@ -115,7 +116,7 @@ public void initializeSecurity(String principal, byte[] token) { */ private void constructUser(String user, byte[] pass) throws KeeperException, InterruptedException { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); ZooReaderWriter zoo = context.getZooReaderWriter(); zoo.putPrivatePersistentData(ZKUserPath + "/" + user, pass, NodeExistsPolicy.FAIL); @@ -154,7 +155,7 @@ public void createUser(String principal, AuthenticationToken token) @Override public void dropUser(String user) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); context.getZooReaderWriter().recursiveDelete(ZKUserPath + "/" + user, NodeMissingPolicy.FAIL); @@ -181,7 +182,7 @@ public void changePassword(String principal, AuthenticationToken token) PasswordToken pt = (PasswordToken) token; if (userExists(principal)) { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(ZKUserPath + "/" + principal); context.getZooReaderWriter().putPrivatePersistentData(ZKUserPath + "/" + principal, ZKSecurityTool.createPass(pt.getPassword()), NodeExistsPolicy.OVERWRITE); diff --git a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java index 8f964e1d4b2..34cb5e3bb95 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java +++ b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java @@ -45,6 +45,7 @@ public class ZKAuthorizor implements Authorizor { private ServerContext context; private String ZKUserPath; private ZooCache zooCache; + private final Object zooCacheLock = new Object(); @Override public void initialize(ServerContext context) { @@ -109,7 +110,7 @@ public void initUser(String user) throws AccumuloSecurityException { @Override public void dropUser(String user) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { ZooReaderWriter zoo = context.getZooReaderWriter(); zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserAuths, NodeMissingPolicy.SKIP); zooCache.clear(ZKUserPath + "/" + user); @@ -132,7 +133,7 @@ public void dropUser(String user) throws AccumuloSecurityException { public void changeAuthorizations(String user, Authorizations authorizations) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); context.getZooReaderWriter().putPersistentData(ZKUserPath + "/" + user + ZKUserAuths, ZKSecurityTool.convertAuthorizations(authorizations), NodeExistsPolicy.OVERWRITE); diff --git a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java index 96bb92079e2..f04b8431515 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java +++ b/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java @@ -59,6 +59,7 @@ public class ZKPermHandler implements PermissionHandler { private String ZKTablePath; private String ZKNamespacePath; private ZooCache zooCache; + private final Object zooCacheLock = new Object(); private final String ZKUserSysPerms = "/System"; private final String ZKUserTablePerms = "/Tables"; private final String ZKUserNamespacePerms = "/Namespaces"; @@ -191,7 +192,7 @@ public void grantSystemPermission(String user, SystemPermission permission) } if (perms.add(permission)) { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserSysPerms, ZKSecurityTool.convertSystemPermissions(perms), NodeExistsPolicy.OVERWRITE); @@ -220,7 +221,7 @@ public void grantTablePermission(String user, String table, TablePermission perm try { if (tablePerms.add(permission)) { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(ZKUserPath + "/" + user + ZKUserTablePerms + "/" + table); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserTablePerms + "/" + table, ZKSecurityTool.convertTablePermissions(tablePerms), NodeExistsPolicy.OVERWRITE); @@ -250,7 +251,7 @@ public void grantNamespacePermission(String user, String namespace, try { if (namespacePerms.add(permission)) { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(ZKUserPath + "/" + user + ZKUserNamespacePerms + "/" + namespace); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserNamespacePerms + "/" + namespace, ZKSecurityTool.convertNamespacePermissions(namespacePerms), @@ -281,7 +282,7 @@ public void revokeSystemPermission(String user, SystemPermission permission) try { if (sysPerms.remove(permission)) { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserSysPerms, ZKSecurityTool.convertSystemPermissions(sysPerms), NodeExistsPolicy.OVERWRITE); @@ -367,7 +368,7 @@ public void revokeNamespacePermission(String user, String namespace, @Override public void cleanTablePermissions(String table) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); for (String user : zooCache.getChildren(ZKUserPath)) { zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserTablePerms + "/" + table, @@ -387,7 +388,7 @@ public void cleanTablePermissions(String table) throws AccumuloSecurityException @Override public void cleanNamespacePermissions(String namespace) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); for (String user : zooCache.getChildren(ZKUserPath)) { zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserNamespacePerms + "/" + namespace, @@ -471,7 +472,7 @@ public void initUser(String user) throws AccumuloSecurityException { */ private void createTablePerm(String user, TableId table, Set perms) throws KeeperException, InterruptedException { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserTablePerms + "/" + table, ZKSecurityTool.convertTablePermissions(perms), NodeExistsPolicy.FAIL); @@ -484,7 +485,7 @@ private void createTablePerm(String user, TableId table, Set pe */ private void createNamespacePerm(String user, NamespaceId namespace, Set perms) throws KeeperException, InterruptedException { - synchronized (zooCache) { + synchronized (zooCacheLock) { zooCache.clear(); zoo.putPersistentData(ZKUserPath + "/" + user + ZKUserNamespacePerms + "/" + namespace, ZKSecurityTool.convertNamespacePermissions(perms), NodeExistsPolicy.FAIL); @@ -494,7 +495,7 @@ private void createNamespacePerm(String user, NamespaceId namespace, @Override public void cleanUser(String user) throws AccumuloSecurityException { try { - synchronized (zooCache) { + synchronized (zooCacheLock) { zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserSysPerms, NodeMissingPolicy.SKIP); zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserTablePerms, NodeMissingPolicy.SKIP); zoo.recursiveDelete(ZKUserPath + "/" + user + ZKUserNamespacePerms, NodeMissingPolicy.SKIP); diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java b/server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java index 70f78451f8e..4450ba46b7f 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java @@ -65,6 +65,8 @@ public class ReplicationTableUtil { public static final String STATUS_FORMATTER_CLASS_NAME = org.apache.accumulo.server.replication.StatusFormatter.class.getName(); + private static final Object lock = new Object(); + private ReplicationTableUtil() {} /** @@ -89,62 +91,65 @@ static synchronized Writer getWriter(ClientContext context) { return replicationTable; } - public synchronized static void configureMetadataTable(AccumuloClient client, String tableName) { - TableOperations tops = client.tableOperations(); - Map> iterators = null; - try { - iterators = tops.listIterators(tableName); - } catch (AccumuloSecurityException | AccumuloException | TableNotFoundException e) { - throw new RuntimeException(e); - } - - if (!iterators.containsKey(COMBINER_NAME)) { - // Set our combiner and combine all columns - // Need to set the combiner beneath versioning since we don't want to turn it off - @SuppressWarnings("deprecation") - var statusCombinerClass = org.apache.accumulo.server.replication.StatusCombiner.class; - IteratorSetting setting = new IteratorSetting(9, COMBINER_NAME, statusCombinerClass); - Combiner.setColumns(setting, Collections.singletonList(new Column(ReplicationSection.COLF))); + public static void configureMetadataTable(AccumuloClient client, String tableName) { + synchronized (lock) { + TableOperations tops = client.tableOperations(); + Map> iterators = null; try { - tops.attachIterator(tableName, setting); + iterators = tops.listIterators(tableName); } catch (AccumuloSecurityException | AccumuloException | TableNotFoundException e) { throw new RuntimeException(e); } - } - // Make sure the StatusFormatter is set on the metadata table - Map properties; - try { - properties = tops.getConfiguration(tableName); - } catch (AccumuloException | TableNotFoundException e) { - throw new RuntimeException(e); - } + if (!iterators.containsKey(COMBINER_NAME)) { + // Set our combiner and combine all columns + // Need to set the combiner beneath versioning since we don't want to turn it off + @SuppressWarnings("deprecation") + var statusCombinerClass = org.apache.accumulo.server.replication.StatusCombiner.class; + IteratorSetting setting = new IteratorSetting(9, COMBINER_NAME, statusCombinerClass); + Combiner.setColumns(setting, + Collections.singletonList(new Column(ReplicationSection.COLF))); + try { + tops.attachIterator(tableName, setting); + } catch (AccumuloSecurityException | AccumuloException | TableNotFoundException e) { + throw new RuntimeException(e); + } + } - for (Entry property : properties.entrySet()) { - if (Property.TABLE_FORMATTER_CLASS.getKey().equals(property.getKey())) { - if (!STATUS_FORMATTER_CLASS_NAME.equals(property.getValue())) { - log.info("Setting formatter for {} from {} to {}", tableName, property.getValue(), - STATUS_FORMATTER_CLASS_NAME); - try { - tops.setProperty(tableName, Property.TABLE_FORMATTER_CLASS.getKey(), + // Make sure the StatusFormatter is set on the metadata table + Map properties; + try { + properties = tops.getConfiguration(tableName); + } catch (AccumuloException | TableNotFoundException e) { + throw new RuntimeException(e); + } + + for (Entry property : properties.entrySet()) { + if (Property.TABLE_FORMATTER_CLASS.getKey().equals(property.getKey())) { + if (!STATUS_FORMATTER_CLASS_NAME.equals(property.getValue())) { + log.info("Setting formatter for {} from {} to {}", tableName, property.getValue(), STATUS_FORMATTER_CLASS_NAME); - } catch (AccumuloException | AccumuloSecurityException e) { - throw new RuntimeException(e); + try { + tops.setProperty(tableName, Property.TABLE_FORMATTER_CLASS.getKey(), + STATUS_FORMATTER_CLASS_NAME); + } catch (AccumuloException | AccumuloSecurityException e) { + throw new RuntimeException(e); + } } - } - // Don't need to keep iterating over the properties after we found the one we were looking - // for - return; + // Don't need to keep iterating over the properties after we found the one we were looking + // for + return; + } } - } - // Set the formatter on the table because it wasn't already there - try { - tops.setProperty(tableName, Property.TABLE_FORMATTER_CLASS.getKey(), - STATUS_FORMATTER_CLASS_NAME); - } catch (AccumuloException | AccumuloSecurityException e) { - throw new RuntimeException(e); + // Set the formatter on the table because it wasn't already there + try { + tops.setProperty(tableName, Property.TABLE_FORMATTER_CLASS.getKey(), + STATUS_FORMATTER_CLASS_NAME); + } catch (AccumuloException | AccumuloSecurityException e) { + throw new RuntimeException(e); + } } } diff --git a/server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java b/server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java index d16948aed80..ced88dd1c3e 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java +++ b/server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java @@ -278,9 +278,9 @@ public List getWorkQueued() throws KeeperException, InterruptedException return children; } - public void waitUntilDone(Set workIDs) throws KeeperException, InterruptedException { + private final Object condVar = new Object(); - final Object condVar = new Object(); + public void waitUntilDone(Set workIDs) throws KeeperException, InterruptedException { Watcher watcher = new Watcher() { @SuppressFBWarnings(value = "NN_NAKED_NOTIFY", diff --git a/server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationKeyTest.java b/server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationKeyTest.java index c8d9e175481..f174729ffb5 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationKeyTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/security/delegation/AuthenticationKeyTest.java @@ -34,6 +34,10 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +@SuppressFBWarnings(value = "NP_NULL_PARAM_DEREF_NONVIRTUAL", + justification = "Test code is testing that a NPE is thrown") public class AuthenticationKeyTest { // From org.apache.hadoop.security.token.SecretManager private static final String DEFAULT_HMAC_ALGORITHM = "HmacSHA1"; diff --git a/server/base/src/test/java/org/apache/accumulo/server/zookeeper/ZooAclUtilTest.java b/server/base/src/test/java/org/apache/accumulo/server/zookeeper/ZooAclUtilTest.java index 31d86a3056e..3b7dbceeb09 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/zookeeper/ZooAclUtilTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/zookeeper/ZooAclUtilTest.java @@ -30,6 +30,10 @@ import org.apache.zookeeper.data.Id; import org.junit.jupiter.api.Test; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +@SuppressFBWarnings(value = "NP_NULL_PARAM_DEREF_NONVIRTUAL", + justification = "Test code is testing that a NPE is thrown") public class ZooAclUtilTest { @Test diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java b/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java index 666970cc09f..1ca47daf4b1 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java @@ -86,6 +86,7 @@ public RecoveryManager(Manager manager, long timeToCacheExistsInMillis) { } private class LogSortTask implements Runnable { + private final Object lock = new Object(); private final String source; private final String destination; private final String sortId; @@ -118,7 +119,7 @@ public void run() { log.warn("Failed to initiate log sort " + source, e); } finally { if (!rescheduled) { - synchronized (RecoveryManager.this) { + synchronized (lock) { closeTasksQueued.remove(sortId); } } diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/AssignmentHandler.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/AssignmentHandler.java index e6154e1392e..33f8d0e59a5 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/AssignmentHandler.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/AssignmentHandler.java @@ -49,6 +49,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + class AssignmentHandler implements Runnable { private static final Logger log = LoggerFactory.getLogger(AssignmentHandler.class); private static final String METADATA_ISSUE = "Saw metadata issue when loading tablet : "; @@ -67,6 +69,7 @@ public AssignmentHandler(TabletServer server, KeyExtent extent, int retryAttempt } @Override + @SuppressFBWarnings("USO_UNSAFE_OBJECT_SYNCHRONIZATION") public void run() { synchronized (server.unopenedTablets) { synchronized (server.openingTablets) { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java index 608291db7af..c6d8da6a220 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java @@ -133,6 +133,7 @@ import com.google.common.cache.Cache; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Scope; @@ -1183,6 +1184,7 @@ static void checkPermission(ServerContext context, TabletHostingServer server, } @Override + @SuppressFBWarnings("USO_UNSAFE_OBJECT_SYNCHRONIZATION") public void loadTablet(TInfo tinfo, TCredentials credentials, String lock, final TKeyExtent textent) { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java index b6fc4695f4f..23a28dab426 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java @@ -168,6 +168,7 @@ import com.google.common.annotations.VisibleForTesting; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Scope; @@ -451,6 +452,7 @@ public MajorCompactor(ServerContext context) { } @Override + @SuppressFBWarnings("USO_UNSAFE_OBJECT_SYNCHRONIZATION") public void run() { while (true) { try { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java index 7b3ec10e86a..005f3051d3e 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java @@ -135,6 +135,7 @@ public class TabletServerResourceManager { private final ServerContext context; private final Cache fileLenCache; + private final Object tabletServerResourceManagerLock = new Object(); /** * This method creates a task that changes the number of core and maximum threads on the thread @@ -664,6 +665,7 @@ public class TabletResourceManager { private final KeyExtent extent; private final AccumuloConfiguration tableConf; + private final Object tabletResourceManagerLock = new Object(); TabletResourceManager(KeyExtent extent, AccumuloConfiguration tableConf) { requireNonNull(extent, "extent is null"); @@ -751,8 +753,8 @@ public void executeMinorCompaction(final Runnable r) { public void close() throws IOException { // always obtain locks in same order to avoid deadlock - synchronized (TabletServerResourceManager.this) { - synchronized (this) { + synchronized (tabletServerResourceManagerLock) { + synchronized (tabletResourceManagerLock) { if (closed) { throw new IOException("closed"); } diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/UnloadTabletHandler.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/UnloadTabletHandler.java index 2000f40b99c..516fb364e8a 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/UnloadTabletHandler.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/UnloadTabletHandler.java @@ -35,6 +35,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + class UnloadTabletHandler implements Runnable { private static final Logger log = LoggerFactory.getLogger(UnloadTabletHandler.class); private final KeyExtent extent; @@ -51,6 +53,7 @@ public UnloadTabletHandler(TabletServer server, KeyExtent extent, TUnloadTabletG } @Override + @SuppressFBWarnings("USO_UNSAFE_OBJECT_SYNCHRONIZATION") public void run() { Tablet t = null; diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java index 14ff551895f..704757596b7 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/memory/NativeMapLoader.java @@ -37,23 +37,26 @@ public class NativeMapLoader { private static final Pattern dotSuffix = Pattern.compile("[.][^.]*$"); private static final String PROP_NAME = "accumulo.native.lib.path"; private static final AtomicBoolean loaded = new AtomicBoolean(false); + private static final Object lock = new Object(); // don't allow instantiation private NativeMapLoader() {} - public synchronized static void load() { - // load at most once; System.exit if loading fails - if (loaded.compareAndSet(false, true)) { - if (loadFromSearchPath(System.getProperty(PROP_NAME)) || loadFromSystemLinker()) { - return; + public static void load() { + synchronized (lock) { + // load at most once; System.exit if loading fails + if (loaded.compareAndSet(false, true)) { + if (loadFromSearchPath(System.getProperty(PROP_NAME)) || loadFromSystemLinker()) { + return; + } + log.error( + "FATAL! Accumulo native libraries were requested but could not" + + " be be loaded. Either set '{}' to false in accumulo.properties or make" + + " sure native libraries are created in directories set by the JVM" + + " system property '{}' in accumulo-env.sh!", + Property.TSERV_NATIVEMAP_ENABLED, PROP_NAME); + System.exit(1); } - log.error( - "FATAL! Accumulo native libraries were requested but could not" - + " be be loaded. Either set '{}' to false in accumulo.properties or make" - + " sure native libraries are created in directories set by the JVM" - + " system property '{}' in accumulo-env.sh!", - Property.TSERV_NATIVEMAP_ENABLED, PROP_NAME); - System.exit(1); } } diff --git a/start/src/main/java/org/apache/accumulo/start/Main.java b/start/src/main/java/org/apache/accumulo/start/Main.java index e736821e215..2d58790377d 100644 --- a/start/src/main/java/org/apache/accumulo/start/Main.java +++ b/start/src/main/java/org/apache/accumulo/start/Main.java @@ -40,6 +40,7 @@ public class Main { private static ClassLoader classLoader; private static Class vfsClassLoader; private static Map servicesMap; + private static final Object lock = new Object(); public static void main(final String[] args) throws Exception { // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when @@ -91,29 +92,32 @@ public static void main(final String[] args) throws Exception { } - public static synchronized ClassLoader getClassLoader() { - if (classLoader == null) { - try { - classLoader = (ClassLoader) getVFSClassLoader().getMethod("getClassLoader").invoke(null); - Thread.currentThread().setContextClassLoader(classLoader); - } catch (IOException | IllegalArgumentException | ReflectiveOperationException - | SecurityException e) { - die(e, "Problem initializing the class loader"); + public static ClassLoader getClassLoader() { + synchronized (lock) { + if (classLoader == null) { + try { + classLoader = (ClassLoader) getVFSClassLoader().getMethod("getClassLoader").invoke(null); + Thread.currentThread().setContextClassLoader(classLoader); + } catch (IOException | IllegalArgumentException | ReflectiveOperationException + | SecurityException e) { + die(e, "Problem initializing the class loader"); + } } + return classLoader; } - return classLoader; } @Deprecated - private static synchronized Class getVFSClassLoader() - throws IOException, ClassNotFoundException { - if (vfsClassLoader == null) { - Thread.currentThread().setContextClassLoader( - org.apache.accumulo.start.classloader.AccumuloClassLoader.getClassLoader()); - vfsClassLoader = org.apache.accumulo.start.classloader.AccumuloClassLoader.getClassLoader() - .loadClass("org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader"); + private static Class getVFSClassLoader() throws IOException, ClassNotFoundException { + synchronized (lock) { + if (vfsClassLoader == null) { + Thread.currentThread().setContextClassLoader( + org.apache.accumulo.start.classloader.AccumuloClassLoader.getClassLoader()); + vfsClassLoader = org.apache.accumulo.start.classloader.AccumuloClassLoader.getClassLoader() + .loadClass("org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader"); + } + return vfsClassLoader; } - return vfsClassLoader; } private static void execKeyword(final KeywordExecutable keywordExec, final String[] args) { @@ -226,11 +230,13 @@ public static void printUsage() { System.out.println(); } - public static synchronized Map getExecutables(final ClassLoader cl) { - if (servicesMap == null) { - servicesMap = checkDuplicates(ServiceLoader.load(KeywordExecutable.class, cl)); + public static Map getExecutables(final ClassLoader cl) { + synchronized (lock) { + if (servicesMap == null) { + servicesMap = checkDuplicates(ServiceLoader.load(KeywordExecutable.class, cl)); + } + return servicesMap; } - return servicesMap; } public static Map diff --git a/start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java b/start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java index 72c76eaa106..c124e2aa72f 100644 --- a/start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java +++ b/start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java @@ -48,6 +48,7 @@ public class AccumuloClassLoader { private static URL accumuloConfigUrl; private static URLClassLoader classloader; private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class); + private static final Object lock = new Object(); static { String configFile = System.getProperty("accumulo.properties", "accumulo.properties"); @@ -193,34 +194,36 @@ private static ArrayList findAccumuloURLs() throws IOException { return urls; } - public static synchronized ClassLoader getClassLoader() throws IOException { - if (classloader == null) { - ArrayList urls = findAccumuloURLs(); - - ClassLoader parentClassLoader = ClassLoader.getSystemClassLoader(); - - log.debug("Create 2nd tier ClassLoader using URLs: {}", urls); - classloader = - new URLClassLoader("AccumuloClassLoader (loads everything defined by general.classpaths)", - urls.toArray(new URL[urls.size()]), parentClassLoader) { - @Override - protected synchronized Class loadClass(String name, boolean resolve) - throws ClassNotFoundException { - - if (name.startsWith("org.apache.accumulo.start.classloader.vfs")) { - Class c = findLoadedClass(name); - if (c == null) { - try { - // try finding this class here instead of parent - findClass(name); - } catch (ClassNotFoundException e) {} - } + public static ClassLoader getClassLoader() throws IOException { + synchronized (lock) { + if (classloader == null) { + ArrayList urls = findAccumuloURLs(); + + ClassLoader parentClassLoader = ClassLoader.getSystemClassLoader(); + + log.debug("Create 2nd tier ClassLoader using URLs: {}", urls); + classloader = new URLClassLoader( + "AccumuloClassLoader (loads everything defined by general.classpaths)", + urls.toArray(new URL[urls.size()]), parentClassLoader) { + @Override + protected synchronized Class loadClass(String name, boolean resolve) + throws ClassNotFoundException { + + if (name.startsWith("org.apache.accumulo.start.classloader.vfs")) { + Class c = findLoadedClass(name); + if (c == null) { + try { + // try finding this class here instead of parent + findClass(name); + } catch (ClassNotFoundException e) {} } - return super.loadClass(name, resolve); } - }; - } + return super.loadClass(name, resolve); + } + }; + } - return classloader; + return classloader; + } } } diff --git a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java index 942fffcb2c2..c9f51968464 100644 --- a/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java +++ b/start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java @@ -432,24 +432,26 @@ public static ClassLoader getContextClassLoader(String contextName) throws IOExc return getContextManager().getClassLoader(contextName); } - public static synchronized ContextManager getContextManager() throws IOException { - if (contextManager == null) { - getClassLoader(); - try { - contextManager = new ContextManager(generateVfs(), () -> { - try { - return getClassLoader(); - } catch (IOException e) { - // throw runtime, then unwrap it. - throw new UncheckedIOException(e); - } - }); - } catch (UncheckedIOException uioe) { - throw uioe.getCause(); + public static ContextManager getContextManager() throws IOException { + synchronized (lock) { + if (contextManager == null) { + + getClassLoader(); + try { + contextManager = new ContextManager(generateVfs(), () -> { + try { + return getClassLoader(); + } catch (IOException e) { + // throw runtime, then unwrap it. + throw new UncheckedIOException(e); + } + }); + } catch (UncheckedIOException uioe) { + throw uioe.getCause(); + } } + return contextManager; } - - return contextManager; } public static void close() { diff --git a/test/src/main/spotbugs/exclude-filter.xml b/test/src/main/spotbugs/exclude-filter.xml index ea7b5384050..3861f32eb19 100644 --- a/test/src/main/spotbugs/exclude-filter.xml +++ b/test/src/main/spotbugs/exclude-filter.xml @@ -35,4 +35,10 @@ + + + + + +