Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expr: refactor for avoid global evaluator usage #818

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

msaf1980
Copy link
Collaborator

Try to refactor expr package for avoid global evaluator usage

@msaf1980 msaf1980 marked this pull request as draft January 24, 2024 14:43
@msaf1980 msaf1980 force-pushed the feat/expr_wo_global branch 3 times, most recently from 21b4f0b to c1b3322 Compare January 24, 2024 15:10
expr/expr.go Outdated Show resolved Hide resolved
@msaf1980 msaf1980 force-pushed the feat/expr_wo_global branch 4 times, most recently from 8d21b2c to fdddd1f Compare February 6, 2024 12:30
@msaf1980 msaf1980 marked this pull request as ready for review February 6, 2024 12:34
@msaf1980
Copy link
Collaborator Author

msaf1980 commented Feb 6, 2024

@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
Copy link
Collaborator

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

@npazosmendez
Copy link
Collaborator

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
Copy link
Collaborator Author

msaf1980 commented Feb 22, 2024

o
Changing signature of Do - is ugly, i think.
Compromise variant is #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
Copy link
Collaborator

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

Copy link
Collaborator

@npazosmendez npazosmendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but another review would be nice since this is a big PR

@msaf1980
Copy link
Collaborator Author

msaf1980 commented Feb 23, 2024

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 #811 not solved all of problems.

@msaf1980 msaf1980 added this pull request to the merge queue Mar 4, 2024
Merged via the queue into go-graphite:main with commit dd8dde6 Mar 4, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants