[GitHub] [accumulo] ctubbsii commented on a change in pull request #2101: Avoid recreating ServiceEnvironment

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2101:
URL: https://github.com/apache/accumulo/pull/2101#discussion_r631362740



##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableUtils.java
##########
@@ -303,9 +312,11 @@ public TableId getTableId() {
 
     Selection selection = selector.select(new CompactionSelector.SelectionParameters() {
 
+      private final ServiceEnvironment senv = new ServiceEnvironmentImpl(tablet.getContext());
+

Review comment:
       This is duplicated in the `selectFiles` method for InitParamaters and for SelectionParameters. It could be created once in the selectFiles method as a final local variable.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
##########
@@ -178,14 +180,17 @@ private ExecutorService createPriorityExecutor(ScanExecutorConfig sec,
         Comparator<ScanInfo> comparator =
             factory.createComparator(new ScanPrioritizer.CreateParameters() {
 
+              private final Supplier<ServiceEnvironment> senvSupplier =
+                  Suppliers.memoize(() -> new ServiceEnvironmentImpl(context));

Review comment:
       It's not obvious here why this instance needs to be lazily loaded when other instances are not.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableUtils.java
##########
@@ -226,14 +227,17 @@ static AccumuloConfiguration createCompactionConfiguration(Tablet tablet,
         cfg.getClassName(), CompactionConfigurer.class);
 
     configurer.init(new CompactionConfigurer.InitParamaters() {
+
+      private final ServiceEnvironment senv = new ServiceEnvironmentImpl(tablet.getContext());
+

Review comment:
       This is created twice in the createCompactionConfiguration method, once for the anonymous InitParamaters (which I just noticed is misspelled; should be InitParameters) and once for the anonymous InputParameters below. This could be created once in the method as a final local variable, and referenced in each anonymous inner class instance's getEnvironment method.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java
##########
@@ -213,6 +216,11 @@ private static ScanDispatcher createScanDispatcher(AccumuloConfiguration conf,
         conf.getAllPropertiesWithPrefixStripped(Property.TABLE_SCAN_DISPATCHER_OPTS);
 
     newDispatcher.init(new ScanDispatcher.InitParameters() {
+
+      // scan dispatcher are in the critical path for scans, so only create ServiceEnv if needed.
+      private final Supplier<ServiceEnvironment> senvSupplier =
+          Suppliers.memoize(() -> new ServiceEnvironmentImpl(context));

Review comment:
       Under which circumstances would this one not be used, and the lazy loading would help?

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
##########
@@ -783,6 +788,11 @@ public void executeReadAhead(KeyExtent tablet, ScanDispatcher dispatcher, ScanSe
       scanExecutors.get("meta").execute(task);
     } else {
       DispatchParameters params = new DispatchParamsImpl() {
+
+        // in scan critical path so only create ServiceEnv if needed
+        private final Supplier<ServiceEnvironment> senvSupplier =
+            Suppliers.memoize(() -> new ServiceEnvironmentImpl(context));

Review comment:
       For my own understanding, under which circumstances would this not actually be used, and therefore not need to be created?




-- 
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