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

[pulsar-sql] Bump presto.version to run PrestoServer on JDK11+ #16163

Merged
merged 10 commits into from
Jun 28, 2022

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Jun 21, 2022

Motivation

This is another attempt to fix #14951 without introduce workaround.

Modifications

Bump presto.version to 334.

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes)

To fix #14951. This version bump should be transparent to users.

Config option presto.version has been removed at trinodb/trino#3708. Since PrestoServer uses strict config validations, users who configure presto.version may get a fatal error when starting PrestoServer.

  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

This is another attempt to fix apache#14951 without introduce workaround.

Signed-off-by: tison <wander4096@gmail.com>
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 21, 2022
Signed-off-by: tison <wander4096@gmail.com>
This refers to trinodb/trino@f4d7de3.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as ready for review June 22, 2022 03:05
@tisonkun
Copy link
Member Author

cc @lhotari @codelipenghui

This change comes from #14951 (comment). And I touch this module since java-version-trim-agent is the only module uses java.util.logging related to this thread.

Bump dependency version is clear and easy to maintain than introducing an in-place agent module IMO.

cc @gaoran10

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

All tests has passed at https://github.com/apache/pulsar/runs/6997463530. The latest commit is about test file changes. So I think CPP task failed on flaky tests.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

tisonkun commented Jun 22, 2022

cc @Technoboy- @eolivelli

@gaoran10
Copy link
Contributor

gaoran10 commented Jun 22, 2022

Great work! LGTM. But there is a similar PR, maybe we could refer to this comment #14953 (review), if we want to upgrade the Trino to 334, it's better to discuss in the dev list, because the Trino 334 needs jdk11.

@tisonkun
Copy link
Member Author

@gaoran10 Thanks for your inputs! I'm not aware of that PR before. Let me investigate a bit and decide whether to start a thread on dev@ or close this PR :)

@tisonkun
Copy link
Member Author

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

at DataStax we upgraded Presto last year, no particular issues so far

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

@eolivelli thanks for your review and information! That sounds great.

ping @gaoran10 @lhotari @codelipenghui

Copy link
Contributor

@MarvinCai MarvinCai left a comment

Choose a reason for hiding this comment

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

lgtm

@tisonkun
Copy link
Member Author

@codelipenghui @eolivelli @lhotari could you give this patch a final review and merge if no further concern?

@codelipenghui codelipenghui merged commit b596a9c into apache:master Jun 28, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 28, 2022
@tisonkun tisonkun deleted the issue-14951 branch June 28, 2022 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulsar SQL won't start with Java 11.0.14.1
5 participants