You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2026/01/12 19:20:50 UTC
[PR] Fix collection concurrency [accumulo]
DomGarguilo opened a new pull request, #6052:
URL: https://github.com/apache/accumulo/pull/6052
Looked through the code for places that use `Collections.syncronized*`. The docs for these collections reads:
```
It is imperative that the user manually synchronize on the returned list when traversing it
```
This PR fixes all the spots in the code that I found that where not iterating within a `synchronized` block.
This PR also simplifies TimeoutTracker creation with computeIfAbsent in batch reader/writer code.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687498931
##########
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:
The javadoc could use some improvements to reduce uncertainty around using forEach/removeIf and/or suggest using them.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687443215
##########
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:
Looking at the javadoc it mentions the following
```
* It is imperative that the user manually synchronize on the returned
* collection when traversing it via {@link Iterator}, {@link Spliterator}
* or {@link Stream}:
```
One way to read that documentation is that manual sync is only needed when doing those three specific operations and all other methods (including forEach) on the interface do not require sync. So maybe the javadoc is kinda correct.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687075810
##########
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:
I did do some manual testing and it does appear .forEach off of a .synchronizedMap() does protect against CME where as swapping in other looping techniques does not so I think you're right.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2686915241
##########
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:
I was looking into this to learn more about it and I actually cannot find info that says forEach is automatically synchronized. Did you see that somewhere?
I was going based off the javadoc which says
> It is imperative that the user manually synchronize on the returned map when traversing any of its collection views via Iterator, Spliterator or Stream:
And I thought forEach just used an Iterator under the hood.
I did see that its possible that some implementations of the forEach method do the synchronization internally but its best to explicitly iterate in a sync block
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2684073968
##########
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:
I did not know that. Thanks for pointing that out. I'll update things in this PR to make use of that where possible.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687367865
##########
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:
Yeah I found that behavior by looking at the java source. It should be in the javadocs though somewhere if its not. I wonder how easy it would be to make a PR to java to update the javadocs.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #6052:
URL: https://github.com/apache/accumulo/pull/6052
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687464123
##########
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:
I did a few searches here https://bugs.java.com/ to see if there were any existing bugs open about the javadoc and did not see anything. I did not look at the javadoc in the latest version of openjdk to see if there were any updates.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687451393
##########
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:
Yea technically its correct but I feel like it would be super helpful to at least point the reader to `forEach` as an alternative.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687075810
##########
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:
I did do some manual testing and it does appear .forEach off of a .synchronizedMap() does protect against CME where as swapping in other looping techniques does not so in practice it appears you're right but everything im seeing still suggests to manually sync
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix collection concurrency [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6052:
URL: https://github.com/apache/accumulo/pull/6052#discussion_r2687075810
##########
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:
I did do some manual testing and it does appear .forEach off of a .synchronizedMap() does protect against CME where as swapping in other looping techniques does not so in practice it appears you're right but everything im seeing still suggests to manually sync
edit:
I think proves you're correct:
https://github.com/openjdk/jdk17/blob/4afbcaf55383ec2f5da53282a1547bac3d099e9d/src/java.base/share/classes/java/util/Collections.java#L2129
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org