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

support running ams in jdk17 environment. #1896

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

yabola
Copy link
Contributor

@yabola yabola commented Aug 29, 2023

Why are the changes needed?

support running Amoro-ams in jdk17 environment.

Brief change log

  • Modify some JVM configurations to adapt to JDK 17

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@yabola yabola changed the title support amoro ams in jdk17 support running Amoro-ams in jdk17 environment. Aug 29, 2023
@yabola yabola changed the title support running Amoro-ams in jdk17 environment. support running ams in jdk17 environment. Aug 29, 2023
@yabola
Copy link
Contributor Author

yabola commented Aug 29, 2023

@baiyangtx
Copy link
Contributor

Will these additional parameters fail to run due to unrecognized parameters under JDK 8?

@yabola
Copy link
Contributor Author

yabola commented Aug 29, 2023

@baiyangtx no, I have also tested in jdk1.8

@wangtaohz
Copy link
Contributor

Thanks for your work! 👍

Here's another optimization for the ams.sh: delete the MaxPermSize-related configuration. This parameter has been invalid since JDK 8, which leads to a warning message during startup.

OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=128m; support was removed in 8.0

It would be better if we could fix it together.

@shidayang
Copy link
Contributor

Thanks for your contributions, I concern if the command like '--add-options' can work in jdk-1.8,

@wangtaohz
Copy link
Contributor

Thanks for your contributions, I concern if the command like '--add-options' can work in jdk-1.8,

I test it locally in my local jdk8 environment, and it works well.

@yabola
Copy link
Contributor Author

yabola commented Aug 29, 2023

I have deleted MaxPermSize config

Copy link
Contributor

@wangtaohz wangtaohz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@baiyangtx baiyangtx left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you for submitting your Pull Request, your contribution is greatly appreciated!

@zhoujinsong zhoujinsong merged commit 55d32d3 into apache:master Aug 29, 2023
@yabola
Copy link
Contributor Author

yabola commented Aug 29, 2023

@baiyangtx @wangtaohz @shidayang @zhoujinsong Thank you for your review!

@wangtaohz wangtaohz mentioned this pull request Sep 7, 2023
56 tasks
ShawHee pushed a commit to ShawHee/arctic that referenced this pull request Dec 29, 2023
* support amoro ams in jdk17

* support ams in jdk17

* remove MAX_PERM_CONFIG

---------

Co-authored-by: chenliang.lu <chenlianglu@tencent.com>
Co-authored-by: baiyangtx <xiangnebula@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants