keith-turner commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2684003185
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/ExternalCompactionExecutor.java:
##########
@@ -198,7 +198,9 @@ ExternalCompactionJob reserveExternalCompaction(long
priority, String compactorI
public Stream<TCompactionQueueSummary> summarize() {
HashSet<Short> uniqPrios = new HashSet<Short>();
- queuedJob.forEach(job -> uniqPrios.add(job.getJob().getPriority()));
+ synchronized (queuedJob) {
+ queuedJob.forEach(job -> uniqPrios.add(job.getJob().getPriority()));
Review Comment:
The forEach method on sync set will sync, so probably do not need to add the
sync.
##########
core/src/main/java/org/apache/accumulo/core/conf/ConfigurationCopy.java:
##########
@@ -74,9 +74,11 @@ public String get(Property property) {
@Override
public void getProperties(Map<String,String> props, Predicate<String>
filter) {
- for (Entry<String,String> entry : copy.entrySet()) {
- if (filter.test(entry.getKey())) {
- props.put(entry.getKey(), entry.getValue());
+ synchronized (copy) {
Review Comment:
Could use `copy.forEach((k,v)->{...})` here. The for each method will
synchronize and also avoids allocating the Entry object. So its probably a
bit more efficient and shorter.
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/system/StatsIterator.java:
##########
@@ -94,8 +94,10 @@ public void report(boolean reportDeepCopies) {
if (reportDeepCopies) {
// recurse down the fat tree of deep copies forcing them to report
- for (var deepCopy : deepCopies) {
- deepCopy.report(true);
+ synchronized (deepCopies) {
Review Comment:
Could use `deepCopies.forEach()` here, it will synchronize and be a bit
shorter.
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/FileCompactor.java:
##########
@@ -255,7 +255,9 @@ private static void updateTotalEntries() {
if (currentTime - lastUpdateTime < Duration.ofMillis(100).toNanos()) {
return;
}
- runningCompactions.forEach(FileCompactor::updateGlobalEntryCounts);
+ synchronized (runningCompactions) {
Review Comment:
Do not need to synchronize when using foreach method on a synchronized
collection, the foreach method will do it.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Migrations.java:
##########
@@ -50,13 +50,15 @@ private void
removeIf(Predicate<Map.Entry<KeyExtent,TServerInstance>> removalTes
DataLevel... levels) {
for (var dataLevel : levels) {
var mmap = migrations.get(dataLevel);
- mmap.entrySet().removeIf(entry -> {
- if (removalTest.test(entry)) {
- log.trace("Removed migration {} -> {}", entry.getKey(),
entry.getValue());
- return true;
- }
- return false;
- });
+ synchronized (mmap) {
+ mmap.entrySet().removeIf(entry -> {
Review Comment:
Should not need to sync when using removeIf on a synchronized collection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
