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

Remove CallContext and its ThreadLocal usage #589

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Dec 24, 2024

Now ready for review. It's a big PR, but there is no functional change here, only removals of CallContext and PolarisCallContext.

Fixes #463.

Removing CallContext is done in 2 commits that can be reviewed separately:

  1. Removal of PolarisCallContext: this component is an aggregation of 3 application-scoped beans, and one request-scoped bean (the meta store session). It is "easy" to replace it by the components that are actually needed, however since it's used nearly everywhere, the blast radius is quite big. Application scoped beans are generally replaced with constructor injection, and request-scoped ones are passed through additional method arguments (except when the component is also request-scoped, in which case constructor injection is used).
  2. Removal of CallContext proper. This is easier after step 1. The only issue is to revisit how PolarisCatalogHandlerWrapper closes the base catalog. The doCatalogOperation trick wasn't being done at the right level imho and could leave some catalogs unclosed in case of runtime errors.

@adutra adutra force-pushed the remove-call-context branch from ea2175d to 08a09f6 Compare December 26, 2024 17:04
@adutra adutra changed the title Remove CallContext and its ThreadLocal usage [WIP] Remove CallContext and its ThreadLocal usage Dec 26, 2024
@adutra adutra force-pushed the remove-call-context branch 17 times, most recently from facd3fd to be93b0e Compare January 9, 2025 14:00
@adutra adutra force-pushed the remove-call-context branch 8 times, most recently from fbf254b to 92f1632 Compare January 13, 2025 20:49
@adutra adutra force-pushed the remove-call-context branch from 92f1632 to 5c18e9c Compare January 14, 2025 10:54
@adutra adutra changed the title [WIP] Remove CallContext and its ThreadLocal usage Remove CallContext and its ThreadLocal usage Jan 14, 2025
@adutra adutra marked this pull request as ready for review January 14, 2025 10:55
@adutra
Copy link
Contributor Author

adutra commented Jan 14, 2025

Ready for review.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Thanks for this very nice cleanup!

@adutra adutra merged commit b84f462 into apache:main Jan 14, 2025
5 checks passed
@adutra adutra deleted the remove-call-context branch January 14, 2025 11:14
@collado-mike
Copy link
Contributor

This was a pretty major design decision, IMO. CallContext is an interface that allows for a lot of extension and flexibility in the runtime. This PR went from "Ready for review" to "merged" in 20 minutes with very little community feedback. There are a lot of implications for things like MDC variables in logs and other context variables that were being set at the start of the request and are no longer accessible to downstream components. I get that the thread local implementation wasn't loved by everyone, but to completely rip out the interface entirely is major.

cc: @jbonofre @adutra @dennishuo @eric-maynard @flyrain @RussellSpitzer @takidau

@snazy
Copy link
Member

snazy commented Jan 18, 2025

@collado-mike I think we all agree that a ThreadLocal is generally not a good choice. The issue tackled here is #463, which has been open for more than two months - nobody commented on that issue. Sure, this PR has been merged quite quickly, maybe too quick. But this PR does nothing else than what #463 describes.

We all explicitly agreed that we want to move to CDI and we all explicitly agreed that we want to move to Quarkus.

#463 also mentions that @RequestScoped injection is also a way to pass information around to the interested consumers.

The documented contract of that interface said: Stores elements associated with an individual REST request such as RealmContext, caller identity/role, authn/authz, etc. . It says nothing about being a public extension point. The factory functions of the interface imply that there is no other information.

The only implementation of that interface documents The Call context is allocated each time a new REST request is processed. It contains instances of low-level services required to process that request`.

@RequestScoped injection with request-scoped producers is a CDI conformant way to pass request-contextual information around.

It generally helps when the documentation are clear about the intent and structure of the code (package naming, module structure) supports/separates the intent and scope.

It would be good to know what exactly is no longer possible with @RequestScoped. I am sure we can find alternatives there.

CC @jbonofre @dimas-b @adutra @takidau @dennishuo @eric-maynard @flyrain @RussellSpitzer

@collado-mike
Copy link
Contributor

collado-mike commented Jan 19, 2025 via email

@RussellSpitzer
Copy link
Member

This needs to be reverted and discussed.

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.

Remove and ban usage of [Inheritable]ThreadLocal
4 participants