-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Nullify DriverFactory#operatorFactories on noMoreDrivers #21706
Nullify DriverFactory#operatorFactories on noMoreDrivers #21706
Conversation
Shouldn't this factories release memory in theirs |
core/trino-main/src/main/java/io/trino/operator/DriverFactory.java
Outdated
Show resolved
Hide resolved
ab9e612
to
4cd5c78
Compare
We could do it but taking this approach (proposed one) addresses potential issues for all operators. |
8748938
to
3e593ef
Compare
3e593ef
to
0018706
Compare
@@ -88,6 +90,7 @@ public OptionalInt getDriverInstances() | |||
return driverInstances; | |||
} | |||
|
|||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be from jakarta.annotation
package
there are maven failures, see https://github.com/trinodb/trino/pull/21706/files#r1580024450 |
3210779
to
c92205a
Compare
It can happen that operator factories live longer than operators what can cause that considerable amount of memory is not freed.
c92205a
to
f740c2e
Compare
Description
It can happen that operator factories live longer than operators what can cause that considerable amount of memory is not freed. Specifically,
PartitionedLookupSourceFactory
(that retained size can be very big) is kept by chain of referenceJoinBridgeManager <- HashBuilderOperator <- operatorFactories <- driverFactory
even if operators are finished.Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: