search

go-graphite / carbonapi

Implementation of graphite API (graphite-web) in golang
Other
308 stars 140 forks source link

expr: refactor for avoid global evaluator usage #818

Closed msaf1980 closed 4 months ago

msaf1980 commented 6 months ago

Try to refactor expr package for avoid global evaluator usage

msaf1980 commented 5 months ago

@carrieedwards @npazosmendez Hi, what you think about this ? Global Evaluator (or store it on created function instanse) is a ugly. Pass evaluator to function in Do method some better, but not ideal also. One reason for do this at now - use evaluator for refetch in moving. But change Do is bad idea.

npazosmendez commented 5 months ago

Hey @msaf1980, thanks for the ping. I will take a look at this later today.

npazosmendez commented 5 months ago

This looks reasonable. I agree that the big refactor of Dos is not ideal, but I can't think of any better options.

One question: did the global evaluator bring you any particular problems? I agree it's not a nice pattern, but I wonder what motivated you in such big refactor.

msaf1980 commented 5 months ago

o Changing signature of Do - is ugly, i think. Compromise variant is https://github.com/go-graphite/carbonapi/pull/811 No Do change, but helper cleaned from global vars (duplicate in two packages expr and helper) And may be better.

Reason for do this - use expr methods in separate product (in future - with multiply storages). Use for configure it with carbonapi app config is bad design (and possible with one storage only). May be in future we separate Fetch and Eval (no refetch on it), but for moving function it's not too easy (we need precalc points step). With this no Do changes is required.

npazosmendez commented 5 months ago

May be in future we separate Fetch and Eval (no refetch on it), but for moving function it's not too easy (we need precalc points step).

There's other cases that are even more tricky, or maybe even impossible. For example aliasQuery

msaf1980 commented 5 months ago

May be in future we separate Fetch and Eval (no refetch on it), but for moving function it's not too easy (we need precalc points step).

There's other cases that are even more tricky, or maybe even impossible. For example aliasQuery

We planned separate in some steps: 1) parse 2) extract side effects (get step and correct from/until (for moving)/aliasQuery need fetches/etc 3) Fetch 4) Evaluate (without fetch in it)

But it's not easy and require a lot of time - get step require changes in Storage API.

May be change Do signnature and pass-in evaluator is a simple solution at now with hard refactor (complete separate Fetch and Eval steps) in future. Compromise variant https://github.com/go-graphite/carbonapi/pull/811 not solved all of problems.