gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005375692
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/QueryRunner.java:
##########
@@ -130,13 +131,14 @@ public class QueryRunner {
private JoinOverFlowMode _joinOverflowMode;
@Nullable
private PhysicalTimeSeriesServerPlanVisitor _timeSeriesPhysicalPlanVisitor;
+ private BooleanSupplier _sendStats;
/**
* Initializes the query executor.
* <p>Should be called only once and before calling any other method.
*/
public void init(PinotConfiguration config, InstanceDataManager
instanceDataManager, HelixManager helixManager,
- ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig) {
+ ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig,
BooleanSupplier sendStats) {
Review Comment:
An engineer (in fact, a mathematician) I respect told me once that his
primary requirement for a module to be considered _good enough_ is to be able
to run several instances of it in the same process. Using singletons with state
goes against that pattern. We need to do it sometimes because we cannot
refactor Pinot right now.
Could we create an integration test with two servers with a different send
stats configuration? In the model you propose, we couldn't.
Also, we cannot use SendStatsPredicate directly in pinot-query-runtime
because it is declared in pinot-server, which depends on pinot-query-runtime.
We could probably move SendStatsPredicate somewhere else, but finding a place
where it _can_ be (in technical terms) and _should_ be (in semantic terms)
doesn't seem trivial.
> Can you think of any strategy to avoid adding this extra parameter in so
many places?
In Java, we have three options: Using thread locals, dependency injection,
or context objects containing all required information. None makes much sense
here, given we would want to change the parameters. If I had to choose one, DI
would probably be the cleanest, but it would also require more changes in the
code. In the past, we discussed that, but we thought it was a low priority high
effort task
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
