You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/06/18 01:36:26 UTC
[GitHub] [accumulo] keith-turner opened a new pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
keith-turner opened a new pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632
…anges
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632#discussion_r454347182
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
##########
@@ -20,34 +20,89 @@
import java.util.HashMap;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.dataImpl.KeyExtent;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
import org.apache.accumulo.core.spi.compaction.CompactionServices;
-import org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner;
import org.apache.accumulo.core.util.NamingThreadFactory;
import org.apache.accumulo.fate.util.Retry;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.tserver.TabletServerResourceManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
+
public class CompactionManager {
private static final Logger log = LoggerFactory.getLogger(CompactionManager.class);
private Iterable<Compactable> compactables;
- private Map<CompactionServiceId,CompactionService> services;
+ private volatile Map<CompactionServiceId,CompactionService> services;
Review comment:
Thanks for the explanation. I think what you have is fine. I was just wondering what is the best way to guard against future changes which could introduce concurrency issues. For example, you used a coding pattern which may not be obvious to a new developer, versus using a concrete type that is less likely to change. Your code is definitely more efficient but it was not initially clear to me why. Perhaps just some comments with your explanation or creating a method to specifically update the map would help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632#discussion_r454564370
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
##########
@@ -20,34 +20,89 @@
import java.util.HashMap;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.dataImpl.KeyExtent;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
import org.apache.accumulo.core.spi.compaction.CompactionServices;
-import org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner;
import org.apache.accumulo.core.util.NamingThreadFactory;
import org.apache.accumulo.fate.util.Retry;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.tserver.TabletServerResourceManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
+
public class CompactionManager {
private static final Logger log = LoggerFactory.getLogger(CompactionManager.class);
private Iterable<Compactable> compactables;
- private Map<CompactionServiceId,CompactionService> services;
+ private volatile Map<CompactionServiceId,CompactionService> services;
Review comment:
I will add a comment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
Posted by GitBox <gi...@apache.org>.
keith-turner merged pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632#discussion_r453879887
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
##########
@@ -20,34 +20,89 @@
import java.util.HashMap;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.dataImpl.KeyExtent;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
import org.apache.accumulo.core.spi.compaction.CompactionServices;
-import org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner;
import org.apache.accumulo.core.util.NamingThreadFactory;
import org.apache.accumulo.fate.util.Retry;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.tserver.TabletServerResourceManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
+
public class CompactionManager {
private static final Logger log = LoggerFactory.getLogger(CompactionManager.class);
private Iterable<Compactable> compactables;
- private Map<CompactionServiceId,CompactionService> services;
+ private volatile Map<CompactionServiceId,CompactionService> services;
Review comment:
Did you consider using a ConcurrentHashMap here? I see you made the map volatile, but was wondering if it would help prevent concurrency issues to have a ConcurrentHashMap as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1632: Fix #1609 dynamically reinitialize compaction services when config ch…
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1632:
URL: https://github.com/apache/accumulo/pull/1632#discussion_r454059617
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
##########
@@ -20,34 +20,89 @@
import java.util.HashMap;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.conf.AccumuloConfiguration;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.dataImpl.KeyExtent;
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
import org.apache.accumulo.core.spi.compaction.CompactionServices;
-import org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner;
import org.apache.accumulo.core.util.NamingThreadFactory;
import org.apache.accumulo.fate.util.Retry;
import org.apache.accumulo.server.ServerContext;
-import org.apache.accumulo.tserver.TabletServerResourceManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
+
public class CompactionManager {
private static final Logger log = LoggerFactory.getLogger(CompactionManager.class);
private Iterable<Compactable> compactables;
- private Map<CompactionServiceId,CompactionService> services;
+ private volatile Map<CompactionServiceId,CompactionService> services;
Review comment:
I am using Map.copyOf() for the maps, so its a volatile pointer to an immutable map. Whenever its updated the entire map is replaced with another immutable map. This pattern is good for maps that are concurrently read a lot and updated infrequently. The internal pointers in concurrent hash map are volatile, making all operations on the map slower. The internal pointers in an immutable map are probably not volatile, just our pointer to the entire map is.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org