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/03/26 14:14:23 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1572: Refactor
use of ServerConfigurationFactory
milleruntime opened a new pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572
Divided this PR into 2 commits:
1 - Deprecates the init method in MemoryManager that took ServerConfigurationFactory as a parameter. Replace it with a method that takes ServerContext. Updated Test to mock ServerContext.
2 - Inline some of the methods of ServerConfigurationFactory into ServerContext to reduce references to it. Removed sychronization on the methods now they are a part of ServerContext because i don't think its needed since each server should only have one instance. This is just a first step into eliminating internal use of ServerConfigurationFactory.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1572:
Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399459647
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/MemoryManager.java
##########
@@ -32,8 +33,22 @@
/**
* Initialize the memory manager.
+ *
+ * @deprecated since 2.1 use {@link #init(ServerContext)}
+ */
+ @Deprecated
+ default void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method scheduled for removal in 3.0.0");
+ }
+
+ /**
+ * Initialize the memory manager.
+ *
+ * @param context
+ * ServerContext passed from the Tablet server
+ * @since 2.1
*/
- void init(ServerConfiguration conf);
+ void init(ServerContext context);
Review comment:
My only concern with this is that context is not public API any more than ServerConfiguration was. The object has already changed from a ServerConfiguration object to a ServerConfigurationFactory object (which implemented ServerConfiguration, just to make this API happy), and now ServerContext.
While it's unlikely that too many people have implemented their own MemoryManager, it is configurable, and this churn in the API isn't good for implementers. I think it would be cool if we could follow up on this PR with one that promotes this interface to SPI, and uses a stable MemoryManagerEnvironment object to inject here in place of the internal ServerContext.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime merged pull request #1572: Refactor use of
ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399469259
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -112,7 +125,26 @@ public synchronized ServerConfigurationFactory getServerConfFactory() {
@Override
public AccumuloConfiguration getConfiguration() {
- return getServerConfFactory().getSystemConfiguration();
+ if (systemConfig == null) {
+ ZooCache propCache = new ZooCache(getZooKeepers(), getZooKeepersSessionTimeOut());
+ systemConfig = new ZooConfiguration(this, propCache, getSiteConfiguration());
+ }
+ return systemConfig;
+ }
+
+ public TableConfiguration getTableConfiguration(TableId id) {
+ return getServerConfFactory().getTableConfiguration(id);
+ }
+
+ public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) {
+ return getServerConfFactory().getNamespaceConfiguration(namespaceId);
+ }
+
+ public DefaultConfiguration getDefaultConfiguration() {
+ if (defaultConfig == null) {
+ defaultConfig = DefaultConfiguration.getInstance();
+ }
+ return defaultConfig;
Review comment:
Perhaps this [method](https://github.com/apache/accumulo/blob/3fd5cad92f9b63ac19e4466f3f2d5237b905262c/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java#L43) is poorly named because it just returns a new object:
```
public static DefaultConfiguration getInstance() {
return new DefaultConfiguration();
}
```
I think it is worth storing a local copy to prevent creating too many identical objects.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1572:
Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399542219
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -112,7 +125,26 @@ public synchronized ServerConfigurationFactory getServerConfFactory() {
@Override
public AccumuloConfiguration getConfiguration() {
- return getServerConfFactory().getSystemConfiguration();
+ if (systemConfig == null) {
+ ZooCache propCache = new ZooCache(getZooKeepers(), getZooKeepersSessionTimeOut());
+ systemConfig = new ZooConfiguration(this, propCache, getSiteConfiguration());
+ }
+ return systemConfig;
+ }
+
+ public TableConfiguration getTableConfiguration(TableId id) {
+ return getServerConfFactory().getTableConfiguration(id);
+ }
+
+ public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) {
+ return getServerConfFactory().getNamespaceConfiguration(namespaceId);
+ }
+
+ public DefaultConfiguration getDefaultConfiguration() {
+ if (defaultConfig == null) {
+ defaultConfig = DefaultConfiguration.getInstance();
+ }
+ return defaultConfig;
Review comment:
The class is documented as a singleton... but that appears to have stopped being the case as of 37f900b7550d6ffe7b39f2ff5cd766912ab09141 ; it should be a singleton. It doesn't need to do all the synchronization that the commit cleaned up, but it should still save and reuse the single instance.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1572:
Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399449457
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -112,7 +125,26 @@ public synchronized ServerConfigurationFactory getServerConfFactory() {
@Override
public AccumuloConfiguration getConfiguration() {
- return getServerConfFactory().getSystemConfiguration();
+ if (systemConfig == null) {
+ ZooCache propCache = new ZooCache(getZooKeepers(), getZooKeepersSessionTimeOut());
+ systemConfig = new ZooConfiguration(this, propCache, getSiteConfiguration());
+ }
+ return systemConfig;
+ }
+
+ public TableConfiguration getTableConfiguration(TableId id) {
+ return getServerConfFactory().getTableConfiguration(id);
+ }
+
+ public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) {
+ return getServerConfFactory().getNamespaceConfiguration(namespaceId);
+ }
+
+ public DefaultConfiguration getDefaultConfiguration() {
+ if (defaultConfig == null) {
+ defaultConfig = DefaultConfiguration.getInstance();
+ }
+ return defaultConfig;
Review comment:
I don't think there's any reason to load this singleton lazily.
```suggestion
return DefaultConfiguration.getInstance();
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1572:
Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399024390
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
Rather than override it or call the other method (which won't do anything, since we never call the deprecated one in the tserver), you could make it a deprecated default method in the interface that throws the exception. The exception message, as well as the deprecation message can say that it is scheduled for removal in 3.0.0. Since default methods don't need to be overridden, people can safely delete their current implementation to prepare for its eventual removal.
However, I'm not sure the replacement accepting a ServerContext is any safer. It's still a leak of an internal type (ServerContext) that is subject to change, into what is effectively SPI. We should probably have some other specific MemoryManagerEnvironment type for this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399252881
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
I like throwing an exception on the default implementation, not sure why I didn't just do that.
Sounds like this interface might be a good candidate to move to the SPI since it deals more with performance and not user data.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399547294
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -112,7 +125,26 @@ public synchronized ServerConfigurationFactory getServerConfFactory() {
@Override
public AccumuloConfiguration getConfiguration() {
- return getServerConfFactory().getSystemConfiguration();
+ if (systemConfig == null) {
+ ZooCache propCache = new ZooCache(getZooKeepers(), getZooKeepersSessionTimeOut());
+ systemConfig = new ZooConfiguration(this, propCache, getSiteConfiguration());
+ }
+ return systemConfig;
+ }
+
+ public TableConfiguration getTableConfiguration(TableId id) {
+ return getServerConfFactory().getTableConfiguration(id);
+ }
+
+ public NamespaceConfiguration getNamespaceConfiguration(NamespaceId namespaceId) {
+ return getServerConfFactory().getNamespaceConfiguration(namespaceId);
+ }
+
+ public DefaultConfiguration getDefaultConfiguration() {
+ if (defaultConfig == null) {
+ defaultConfig = DefaultConfiguration.getInstance();
+ }
+ return defaultConfig;
Review comment:
Yeah and since it anything loading the class will use it, I think it could just be created once during class load as a static variable.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1572:
Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399458608
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/MemoryManager.java
##########
@@ -32,8 +33,22 @@
/**
* Initialize the memory manager.
+ *
+ * @deprecated since 2.1 use {@link #init(ServerContext)}
Review comment:
I'd add the note about scheduled for removal in this javadoc tag also.
```suggestion
* @deprecated since 2.1 use {@link #init(ServerContext)}; to be removed in 3.0.0
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r398673918
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
Another option for this update would be to call the new init method:
```init(((ServerConfigurationFactory) conf).getServerContext());```
But I put the exception to prevent calling this in Accumulo. If a user extends this class, I don't think they should be calling this method. The init currently gets called in TabletServerResourceManager [here](https://github.com/apache/accumulo/blob/94844b3ef88bc9e02b5cb5c8eeff81411ac8221d/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java#L432).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399455150
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
Also, I could make the MemoryManager change in a separate PR, since its the only contentious change 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r398606997
##########
File path: server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java
##########
@@ -140,13 +140,6 @@ public TableId getTableId() {
return tableId;
}
- /**
- * returns the actual NamespaceConfiguration that corresponds to the current parent namespace.
- */
- public NamespaceConfiguration getNamespaceConfiguration() {
- return context.getServerConfFactory().getNamespaceConfiguration(parent.namespaceId);
- }
-
Review comment:
Method was never used.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399454328
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
I made the change in 1824acd but I made the new method not have a default impl. My thinking was that since we won't have a way to make them backward compatible, it would be better to error during compilation than runtime in the tserver.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r398673918
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/LargestFirstMemoryManager.java
##########
@@ -121,18 +122,16 @@ public void remove(Long key) {
}
}
- LargestFirstMemoryManager(long maxMemory, int maxConcurrentMincs, int numWaitingMultiplier) {
- this();
- this.maxMemory = maxMemory;
- this.maxConcurrentMincs = maxConcurrentMincs;
- this.numWaitingMultiplier = numWaitingMultiplier;
+ @Override
+ public void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method, no longer should be used.");
Review comment:
An option for this update would be to call the new init method:
```init(((ServerConfigurationFactory) conf).getServerContext());```
But I put the exception to prevent calling this in Accumulo. If a user extends this class, I don't think they should be calling this method. The init currently gets called in TabletServerResourceManager [here](https://github.com/apache/accumulo/blob/94844b3ef88bc9e02b5cb5c8eeff81411ac8221d/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java#L432).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] milleruntime commented on a change in pull request
#1572: Refactor use of ServerConfigurationFactory
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1572: Refactor use of ServerConfigurationFactory
URL: https://github.com/apache/accumulo/pull/1572#discussion_r399470557
##########
File path: server/base/src/main/java/org/apache/accumulo/server/tabletserver/MemoryManager.java
##########
@@ -32,8 +33,22 @@
/**
* Initialize the memory manager.
+ *
+ * @deprecated since 2.1 use {@link #init(ServerContext)}
+ */
+ @Deprecated
+ default void init(ServerConfiguration conf) {
+ throw new IllegalStateException("Deprecated method scheduled for removal in 3.0.0");
+ }
+
+ /**
+ * Initialize the memory manager.
+ *
+ * @param context
+ * ServerContext passed from the Tablet server
+ * @since 2.1
*/
- void init(ServerConfiguration conf);
+ void init(ServerContext context);
Review comment:
I will back out the changes to MemoryManager (which won't be hard since I separated the commits) for something better in another 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services