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:
[email protected]
