You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/04/06 19:25:29 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3278: remove unnecessary synchronization
EdColeman opened a new pull request, #3278:
URL: https://github.com/apache/accumulo/pull/3278
The map used to collect work does not need to be wrapped in a synchronized collection.
The map was being synchronized by 1) wrapping with synchronized collection and also synchronized on the map during access, only one synchronization method should be necessary,
--
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
[GitHub] [accumulo] cshannon commented on pull request #3278: remove unnecessary synchronization
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3278:
URL: https://github.com/apache/accumulo/pull/3278#issuecomment-1500170218
Taking away the synchronized wrapper on the map also requires adding a synchronization block on remove which isn't currently being done, so without it the remove would be unprotected, See https://github.com/apache/accumulo/blob/7f49667f1347ba446d0f8e31271df8ba7ee48397/server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java#L127
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3278: remove unnecessary synchronization
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3278:
URL: https://github.com/apache/accumulo/pull/3278#discussion_r1160252473
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java:
##########
@@ -61,8 +61,8 @@ public class LogSorter {
VolumeManager fs;
AccumuloConfiguration conf;
- private final Map<String,LogProcessor> currentWork =
- Collections.synchronizedMap(new HashMap<String,LogProcessor>());
+ // access must synchronize on currentWork to guard against concurrent updates
+ private final Map<String,LogProcessor> currentWork = new HashMap<>();
Review Comment:
Unless we're fixing a bug, I'd rather not risk introducing a new bug in 1.10 by changing concurrency assumptions in a patch release. Is this actually a bug, or just unneeded? I don't think it's a performance issue in here.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3278: remove unnecessary synchronization
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3278:
URL: https://github.com/apache/accumulo/pull/3278#discussion_r1160275112
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java:
##########
@@ -61,8 +61,8 @@ public class LogSorter {
VolumeManager fs;
AccumuloConfiguration conf;
- private final Map<String,LogProcessor> currentWork =
- Collections.synchronizedMap(new HashMap<String,LogProcessor>());
+ // access must synchronize on currentWork to guard against concurrent updates
+ private final Map<String,LogProcessor> currentWork = new HashMap<>();
Review Comment:
I set the base to 3.0 - we could included it in 2.1.1, but it seems that would have the same objection.
--
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
[GitHub] [accumulo] EdColeman closed pull request #3278: remove unnecessary synchronization
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman closed pull request #3278: remove unnecessary synchronization
URL: https://github.com/apache/accumulo/pull/3278
--
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
[GitHub] [accumulo] EdColeman commented on pull request #3278: remove unnecessary synchronization
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3278:
URL: https://github.com/apache/accumulo/pull/3278#issuecomment-1500290364
Changing base brought in additional changes - closing and will resubmit new PR
--
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