refactor(batch): let QueryManager hold FrontendEnv #3347
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3347 +/- ##
=======================================
Coverage 73.67% 73.68%
=======================================
Files 760 760
Lines 104133 104111 -22
=======================================
- Hits 76723 76709 -14
+ Misses 27410 27402 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
The reason we put |
|
I see, let me consider another approach. The possible inconsistency of the env variables |
|
Now this version uses |
|
Can this be done in the opposite way by putting Arc<FrontendEnv> in QueryManager? |
| } | ||
|
|
||
| pub fn server_address(&self) -> &HostAddr { | ||
| &self.server_addr | ||
| } | ||
|
|
||
| pub async fn query_manager(&self) -> &QueryManager { |
There was a problem hiding this comment.
It's really wierd to me to introduce circular dependency here 😂.
There was a problem hiding this comment.
I think the goal of this pr:
Simplifying fields of
QueryManagerto avoid misuse in different contexts.
is not correct enginnering practice. When we design a component, we should only consider its dependencies, and avoid introducing extra unnecessary dependencies and unnessary assumptions.
There was a problem hiding this comment.
QueryManager have Ref to fields in FrontendEnv, and also has context, i.e. a FrontendEnv, as the function parameters when someone calls schedule and schedule_single.
My intention is to remove one of them so they won't mismatch with each other, I am totally fine with other approaches
Calling with context seems a bit error-prone unless we make QueryManager stateless,
open to discussion
There was a problem hiding this comment.
I prefer to remove the context in QueryManager's schedule methods.
There was a problem hiding this comment.
But I don't want store whole FrontedEnv in QueryManager since this introduces two problems:
- Add unnecessary depencies to
QueryManager - Introduce circular dependencies.
|
Let me create an issue for this and discuss it later, close this for now. |
What's changed and what's your intention?
QueryManageris lightweight as it is just holding severalArcreferring to fields contained inFrontendEnv. And it is very likely thatQueryManagerwill have more and more fields referring to fields inFrontendEnv. Let's just replace them with a singleFrontendEnv.contextin functions, i.e.scheduleandschedule_single, is not being used now and it will not be needed to pass into these two functions anymore after this change. Previously,QueryManageris held by theFrontendEnvwhich in turn is held bycontext. We now make sure aQueryManagerwill not be mismatched with acontextto which it does not belong.Note that this change effectively makesQueryManageronly responsible for the execution of a single query. This is fine for now as currently, no role requires a view of all the queries.By implementing lazy initialization, the
QueryManageris still held byFrontendEnvand manages all the batch queries.Checklist
- [ ] I have written necessary docs and comments- [ ] I have added necessary unit tests and integration tests./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)