clintropolis commented on a change in pull request #10366:
URL: https://github.com/apache/druid/pull/10366#discussion_r500129877
##########
File path:
server/src/main/java/org/apache/druid/server/coordination/ServerManager.java
##########
@@ -117,7 +117,7 @@ public ServerManager(
this.cacheConfig = cacheConfig;
this.segmentManager = segmentManager;
- this.joinableFactory = joinableFactory;
+ this.joinableFactoryWrapper = new JoinableFactoryWrapper(joinableFactory);
Review comment:
`JoinableFactoryWrapper` still doesn't seem quite right. These walkers
all take a `JoinableFactory` just to make a `JoinableFactoryWrapper` in the
constructor. `JoinableFactoryWrapper` doesn't seem to have any state of its
own, why not just make it in the joinable module and inject it directly? Or
maybe it didn't really need to change from static methods?
Regardless, it doesn't need to change in this PR, we can refactor this in
the future, since it feels like maybe there is also another refactor lurking
that I haven't quite figured out in wrapping `CacheConfig`, `CachePopulator`,
`Cache` (and maybe `CacheKeyManager` for brokers) into some tidy package to
handle caching stuffs for walkers with implementations that do the right thing
for where they are running.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
