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

Move constant to relevant scope #13391

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

lukasz-walkiewicz
Copy link
Member

@lukasz-walkiewicz lukasz-walkiewicz commented Jul 28, 2022

Description

Move constant to relevant scope

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@martint
Copy link
Member

martint commented Jul 28, 2022

Improvement

What problem is this solving? What's the impact of loading vs not loading those classes? Presumably, and unless there are resources being initialized in a static context, there should be no effect from loading the classes eagerly.

@lukasz-walkiewicz
Copy link
Member Author

Thanks @martint for taking a look.
This fix is to defer loading Rubix classes and ensure that Trino loads them only when hive.cache.enabled=true. CachingPrestoDistributedFileSystem was being loaded even with caching disabled just because RubixModule is registed as conditional module.

@lukasz-walkiewicz
Copy link
Member Author

The general idea here is to be able to safely remove Rubix dependency if caching is not being used. Rubix is marked by security scanners as vulnarable to https://nvd.nist.gov/vuln/detail/CVE-2022-25647 due to it's dependency on gson. No updates that could fix that at the moment.

raunaqmorarka
raunaqmorarka previously approved these changes Jul 29, 2022
@raunaqmorarka
Copy link
Member

@lukasz-walkiewicz do you have confirmation that a build with this change will pass security scan ?

@lukasz-walkiewicz
Copy link
Member Author

No, I don't but I can confirm that with this change you can safely remove Rubix jar from hive plugin directory so I think it'll pass the scan.

@kokosing
Copy link
Member

kokosing commented Aug 4, 2022

Please rename commit title and PR title and description to Move constant to relevant scope.

@martint
Copy link
Member

martint commented Aug 4, 2022

This change may have the side effect of delaying loading of those classes, but that cannot be guaranteed now or ever, so it shouldn't be the main purpose of this change. It's fine as a code cleanup, though.

@lukasz-walkiewicz
Copy link
Member Author

Updated commit message, thanks for the review.

@kokosing
Copy link
Member

kokosing commented Aug 5, 2022

Please do the same with PR title and description.

@lukasz-walkiewicz lukasz-walkiewicz changed the title Delay loading Rubix classes Move constant to relevant scope Aug 5, 2022
@kokosing kokosing merged commit ba858c7 into trinodb:master Aug 5, 2022
@github-actions github-actions bot added this to the 393 milestone Aug 5, 2022
@lukasz-walkiewicz lukasz-walkiewicz deleted the lazy-rubix-loading branch August 7, 2022 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants