search

thanos-io / promql-engine

Multi-threaded PromQL engine implementation based on the Volcano paper.
Apache License 2.0
131 stars 52 forks source link

Allow distributed engine to have custom optimizers #434

Closed pedro-stanaka closed 3 months ago

pedro-stanaka commented 3 months ago

Summary

The distributed engine constructor overrides any logical optimizers passed to it, in this PR I am appending the optimizers to the existing ones in the options.

MichaHoffmann commented 3 months ago

I think the distributed engine should only use the whitelist of distributed optimizers; that way it can assume it only started with the "common" logical nodes.

MichaHoffmann commented 3 months ago

Generally it looks good. I am wondering is there a usecase to change/remove the default optimizer applied in this case? Like the DistributeAvgOptimizer and DistributedExecutionOptimizer

But without DistributedExecutionOptimizer the engine doesnt distribute anymore

fpetkovski commented 3 months ago

Maybe we should reserve this constructor as a prebaked engine where optimizers cannot be customized. In case we need more customizations, we can always use engine.New and add wanted distributed optimizers. In practice I think we need different engine.Opts type for local and distributed engine to make sure options cannot be mixed.

pedro-stanaka commented 3 months ago

I am going to close this one as per Filip's comment, the Distributed engine is merely an "alias" for the engine, so projects using this project as a library can defer to the New constructor and pass the optimizers from the distributed engine.

As a follow up we might consider cleaning up the distributed engine constructor and struct since the field "endpoints" is unused right now. I will propose that in a follow up PR.