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.

Sign up for GitHub

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

Jump to bottom

Modernize tempto #283

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Modernize tempto #283

merged 1 commit into from
Jan 23, 2025

Conversation

aaneja
Copy link
Contributor

@aaneja aaneja commented Dec 17, 2024 •
edited
Loading

  • Upgrade Gradle to 8.11 (most recent version that works with JDK8)
  • Update core dependencies to more recent versions
  • Remove unused NameService/NameServiceDescriptor implementations
  • Bump version to 1.54-SNAPSHOT

Tested that this upgraded Tempto works with existing presto master -
prestodb/presto#24384

@wanglinsong
Copy link
Member

#284

@aaneja aaneja force-pushed the gradleUpgrade810_jdk8 branch from 031d33f to 87f7bba Compare January 17, 2025 07:02
aaneja added a commit to aaneja/presto that referenced this pull request Jan 17, 2025
@aaneja
Tempto upgrade changes
2b32e48 
prestodb/tempto#283
@aaneja aaneja mentioned this pull request Jan 17, 2025
Open
aaneja
aaneja commented Jan 17, 2025
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
aaneja
aaneja commented Jan 17, 2025
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@aaneja aaneja marked this pull request as ready for review January 17, 2025 13:23
@aaneja aaneja changed the title [DRAFT] Modernize tempto Modernize tempto Jan 17, 2025
@aaneja aaneja force-pushed the gradleUpgrade810_jdk8 branch from 87f7bba to 085e21e Compare January 21, 2025 07:08
ZacBlanco
ZacBlanco suggested changes Jan 21, 2025
View reviewed changes
Copy link

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

Just some small things. Thanks for this effort!

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated
email 'EB230060 aat teradata doot com'
id = 'prestodb'
name = 'PrestoDb'
email = '[email protected]'
Copy link

Choose a reason for hiding this comment

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

does this email actually exist?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. Otherwise, we can create it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can drop this block - I don't see it on our Presto pom's, e.g https://repo1.maven.org/maven2/com/facebook/presto/presto-cli/0.290/presto-cli-0.290.pom

jitpack.yml Outdated
- sdk install java 8.0.422-tem
- sdk use java 8.0.422-tem
install:
- ./gradlew PskipSigning=true -Ptempto_group=com.github.aaneja.tempto clean publishToMavenLocal
Copy link

Choose a reason for hiding this comment

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

Suggested change
- ./gradlew PskipSigning=true -Ptempto_group=com.github.aaneja.tempto clean publishToMavenLocal
- ./gradlew -PskipSigning=true -Ptempto_group=io.prestodb.tempto clean publishToMavenLocal

Copy link

Choose a reason for hiding this comment

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

Actually, tempto_group should probably be com.github.prestodb.tempto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use my own groupid for testing. Since this is jitpack related, it will have no bearing on the actual release artifacts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will delete this file

@aaneja aaneja mentioned this pull request Jan 22, 2025
Open
@aaneja aaneja force-pushed the gradleUpgrade810_jdk8 branch from 085e21e to 67b2ee1 Compare January 22, 2025 10:35
imjalpreet
imjalpreet reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@aaneja thank you! Just some minor clarifications, otherwise LGTM!

Also, I just noticed tempto is still using io.prestodb.* package names. Maybe in a separate effort we can refactor it to align with other presto packages to use com.facebook.presto.*

gradlew Outdated Show resolved Hide resolved
gradlew.bat Show resolved Hide resolved
tempto-core/build.gradle Outdated Show resolved Hide resolved
tempto-runner/src/main/java/io/prestodb/tempto/runner/TemptoRunner.java Show resolved Hide resolved
@aaneja
- Upgrade Gradle to 8.11 (most recent version that works with JDK8)
e6f70ca 
- Update core dependencies to more recent versions
- Remove unused NameService/NameServiceDescriptor implementations
- Bump version to 1.54-SNAPSHOT
@aaneja aaneja force-pushed the gradleUpgrade810_jdk8 branch from 67b2ee1 to e6f70ca Compare January 23, 2025 14:55
@tdcmeehan tdcmeehan merged commit 9263c07 into prestodb:master Jan 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wanglinsong wanglinsong wanglinsong left review comments

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@imjalpreet imjalpreet imjalpreet approved these changes

@ZacBlanco ZacBlanco ZacBlanco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aaneja @wanglinsong @ZacBlanco @imjalpreet @tdcmeehan