zstan commented on code in PR #3076:
URL: https://github.com/apache/ignite-3/pull/3076#discussion_r1463610061
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -324,7 +337,7 @@ public CompletableFuture<ExecutionTarget>
forSystemView(ExecutionTargetFactory f
};
var mappingService = new MappingServiceImpl(
- nodeName, executionTargetProvider, CACHE_FACTORY,
PLAN_CACHE_SIZE, taskExecutor
+ nodeName, executionTargetProvider, CACHE_FACTORY,
planCacheSize, taskExecutor
Review Comment:
it`s out of scope but - why we use planCacheSize as a parameter for
MappingServiceImpl ? seems we need an issue here or change it under the current.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -257,19 +266,23 @@ public SqlQueryProcessor(
public synchronized CompletableFuture<Void> start() {
var nodeName = clusterSrvc.topologyService().localMember().name();
- taskExecutor = registerService(new QueryTaskExecutorImpl(nodeName));
+ taskExecutor = registerService(new QueryTaskExecutorImpl(nodeName,
nodeCfg.planner().threadCount().value()));
Review Comment:
but what if we want to add additional configuration param ? i such a case we
will need to "materialize" all such params, can we push whole configuration
into this method ? And of course - change for appropriate mocks in tests )
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -257,19 +266,23 @@ public SqlQueryProcessor(
public synchronized CompletableFuture<Void> start() {
var nodeName = clusterSrvc.topologyService().localMember().name();
- taskExecutor = registerService(new QueryTaskExecutorImpl(nodeName));
+ taskExecutor = registerService(new QueryTaskExecutorImpl(nodeName,
nodeCfg.planner().threadCount().value()));
var mailboxRegistry = registerService(new MailboxRegistryImpl());
SqlClientMetricSource sqlClientMetricSource = new
SqlClientMetricSource(openedCursors::size);
metricManager.registerSource(sqlClientMetricSource);
+ int planCacheSize =
clusterCfg.planner().estimatedNumberOfQueries().value();
+
var prepareSvc = registerService(PrepareServiceImpl.create(
nodeName,
- PLAN_CACHE_SIZE,
+ planCacheSize,
Review Comment:
this is strange - you "materialize" planCacheSize from clusterCfg and
simultaneously push this config into method. Ok it used near but it strange
usage and as for me expansion of input parameters is evil
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -229,7 +234,9 @@ public SqlQueryProcessor(
CatalogManager catalogManager,
MetricManager metricManager,
SystemViewManager systemViewManager,
- PlacementDriver placementDriver
+ PlacementDriver placementDriver,
Review Comment:
i know one guy who don`t like huge amount of parameters in constructor )))
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
