Consolidate CallContext with PolarisCallContext #1806
Conversation
flyrain
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
jackye1995,
jbonofre,
pingtimeout,
singhpk234,
snazy,
takidau and
vvcephei
as code owners
| this.metaStore = metaStore; | ||
| this.diagServices = diagServices; | ||
| this.configurationStore = new PolarisConfigurationStore() {}; | ||
| this.clock = Clock.system(ZoneId.systemDefault()); |
There was a problem hiding this comment.
This was a copy from the deprecated method, see line 90. I'm OK to change it to UTC. I'd suggest to make that change in a separate PR, as it isn't related to the refactor in this PR.
There was a problem hiding this comment.
Ok to defer, but we must not forget... IIRC we already have a mix of clocks in the codebase. It'd be nice to have a uniform pattern.
| * low-level services required to process that request | ||
| */ | ||
| public class PolarisCallContext { | ||
| public class PolarisCallContext implements CallContext { |
There was a problem hiding this comment.
Just curious, why not go the other way? It seems like it would be easier to have CallContext extend PolarisCallContext, since it can immediately implement all the methods just by delegating to the current PolarisCallContext
There was a problem hiding this comment.
I think that's also a nice idea, however, it will need to change CallContext from an interface to a class, which isn't trivial -- all anonymous classes have to go. Besides, it's reasonable to keep CallContext as an interface, while PolarisCallContext as an impl. I also list the step 3 which promotes methods in PolarisCallContext to the interface CallContext, like getMetaStore(). This will even clean up more, so that, PolarisCallContext doesn't have to return itself from the method getPolarisCallContext().
adutra
left a comment
adutra
left a comment
There was a problem hiding this comment.
The changes LGTM and definitely bring a nice simplification.
I just think that there is one last follow-up step that we should consider: getting rid of CallContext and PolarisCallContext completely.
As you know, this was attempted before, but I'll admit it was done too hastily. But the intent is still relevant imho: CallContext is progressively becoming an empty interface, and PolarisCallContext does not have any business logic attached to it.
Moreover, PolarisCallContext has 3 application-scoped beans, but also one non-CDI bean that is realm-dependent. Because of that, PolarisCallContext itself must be in request scope. Besides, not all injectees require access to all of these beans. By de-entangling those beans and injecting them one by one, we would loosen the coupling between components and maybe even promote some beans from request scope to application scope.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
+1, let's keep improving on it. |
flyrain
dismissed stale reviews from eric-maynard, dimas-b, and adutra
via
c4a9c44
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
After pr #1806, the original constructor is marked as deprecated and is not suppose to be used anymore. This PR removes the usage of the old constructors.
This PR is a follow-up of #1806 to remove all usage of CallContext.of() in tests and the method itself
5 participants
This is the first step to consolidate call context. After this change, you can already use
PolarisCallContextto get aRealmContext. To finish the consolidation, here are follow-ups.PolarisCallContextextendCallContextCallContext.of(), then remove the method.PolarisCallContextto the interfaceCallContext, likegetMetaStore()CallContextinstead ofPolarisCallContextas much as possible. For example,EntitiesResult createEntitiesIfNotExist( @Nonnull PolarisCallContext callCtx, ...)