Adding Presto Version Class and version checks for session properties#17760
Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom Jun 9, 2022
Merged
Conversation
|
|
ajaygeorge
requested changes
May 13, 2022
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
...to-session-property-managers/src/main/java/com/facebook/presto/session/SessionMatchSpec.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
e188c66 to
68b78af
Compare
vaishnavibatni
changed the title
Contributor
|
@vaishnavibatni can you please sign the CLA |
ajaygeorge
reviewed
May 17, 2022
Contributor
ajaygeorge
left a comment
ajaygeorge
left a comment
There was a problem hiding this comment.
Comments around equals and compareTo
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
68b78af to
2fc8ef5
Compare
ajaygeorge
previously requested changes
May 19, 2022
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
2 times, most recently
from
7005927 to
14ace02
Compare
ajaygeorge
reviewed
May 20, 2022
Contributor
ajaygeorge
left a comment
ajaygeorge
left a comment
There was a problem hiding this comment.
Some comments on the Test case
presto-common/src/test/java/com/facebook/presto/common/TestPrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/test/java/com/facebook/presto/common/TestPrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/test/java/com/facebook/presto/common/TestPrestoVersion.java
Outdated
Show resolved
Hide resolved
presto-common/src/test/java/com/facebook/presto/common/TestPrestoVersion.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
14ace02 to
a74af9b
Compare
adkri
reviewed
May 20, 2022
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
a74af9b to
081b2e5
Compare
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
081b2e5 to
2c5043f
Compare
swapsmagic
previously requested changes
May 25, 2022
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
...-property-managers/src/main/java/com/facebook/presto/session/FileSessionPropertyManager.java
Outdated
Show resolved
Hide resolved
swapsmagic
reviewed
May 25, 2022
...to-session-property-managers/src/main/java/com/facebook/presto/session/SessionMatchSpec.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
2c5043f to
5b45520
Compare
tdcmeehan
requested changes
May 25, 2022
presto-common/src/main/java/com/facebook/presto/common/PrestoVersion.java
Outdated
Show resolved
Hide resolved
...to-session-property-managers/src/main/java/com/facebook/presto/session/SessionMatchSpec.java
Outdated
Show resolved
Hide resolved
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
5b45520 to
13a5551
Compare
tdcmeehan
approved these changes
Jun 1, 2022
swapsmagic
reviewed
Jun 1, 2022
Contributor
There was a problem hiding this comment.
where do we need this dependency?
Contributor
There was a problem hiding this comment.
don't see it being used anywhere.
Contributor
Author
There was a problem hiding this comment.
I get this maven error when I remove them:
[ERROR] Failed to execute goal com.facebook.presto:presto-maven-plugin:0.4:check-spi-dependencies (default-check-spi-dependencies) on project presto-session-property-managers:
[ERROR] Presto plugin dependency org.openjdk.jol:jol-core must have scope 'provided'. It is part of the SPI and will be provided at runtime.
tdcmeehan
dismissed stale reviews from swapsmagic and ajaygeorge
Approved offline
vaishnavibatni
changed the title
Summary: We need to compare Presto Versions for Session property version checks to allow only the valid session properties based on the current running Presto version. To do that, I added the Presto Version class in `presto-common` repo so that it can be used by other classes if needed. Test Plan: - Updated vll1_verifier1 - Canaried configerator configs and tested the changes. Reviewers: Subscribers: Tasks: T115110477 Tags:
vaishnavibatni
force-pushed
the
version_control_session_properties
branch
from
13a5551 to
b5a99a7
Compare
ajaygeorge
approved these changes
Jun 8, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary: We need to compare Presto Versions for Session property version checks to allow only the valid session properties based on the current running Presto version. To do that, I added the Presto Version class in
presto-commonrepo so that it can be used by other classes if needed.Test Plan:
- Updated vll1_verifier1
- Canaried configerator configs and tested the changes.
Tasks: T115110477
== RELEASE NOTES ==
General Changes