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

Issue #5492 - Adding java.features.* start properties #5494

Merged
merged 6 commits into from Feb 18, 2021
Merged

Issue #5492 - Adding java.features.* start properties #5494

merged 6 commits into from Feb 18, 2021

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 22, 2020

  • Simplifying alpn start modules in the process.
    (from 11 to 3)

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

@joakime
Issue #5492 - Adding java.features.* start properties
4ddd1a1 
+ Simplifying alpn start modules in the process.
  (from 11 to 3)

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime self-assigned this Oct 22, 2020
@joakime joakime added this to In progress in Jetty 9.4.34 via automation Oct 22, 2020
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.34 Oct 22, 2020
@joakime joakime linked an issue Oct 22, 2020 that may be closed by this pull request
Add ability to manage start modules by java feature #5492
Closed
@joakime joakime requested review from gregw and sbordet October 22, 2020 17:22
@janbartel
Copy link
Contributor

janbartel commented Oct 27, 2020 •
edited

I'm not convinced that we should be using the prefix "java." at all - the System properties set by the jvm use that prefix. Why can't these be "jetty.feature." names, the value of which happens to be based on the version of the jvm.

@gregw gregw removed this from Review in progress in Jetty 9.4.34 Nov 2, 2020
@joakime
Copy link
Contributor Author

joakime commented Feb 15, 2021

I'm not convinced that we should be using the prefix "java." at all - the System properties set by the jvm use that prefix. Why can't these be "jetty.feature." names, the value of which happens to be based on the version of the jvm.

I changed this to runtime.feature.* instead.
That work?

@joakime
Issue #5492 - Updating jetty-start property test expectations
7ed68d0 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@sbordet
Copy link
Contributor

sbordet commented Feb 16, 2021

I would have kept the prefix java.feature because that's what ALPN is.
If tomorrow we add loom or records, then that's what they are, Java features.

I don't like runtime.feature -- it does not convey that it's the Java ALPN APIs we are looking for ("runtime" of what? Java? Jetty? JVM? OS?)

I can live with jetty.feature.alpn.

sbordet
sbordet requested changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

See comment -- I'd leave java.feature.*.

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java Outdated
// In Jetty 10+ these will always be true, but still need to stick around for users that
// want to move between Jetty 9.4.x and 10.0.x+
properties.setProperty("runtime.feature.alpn", Boolean.toString(isMethodAvailable(javax.net.ssl.SSLParameters.class, "getApplicationProtocols", null)), source);
properties.setProperty("runtime.feature.jpms", Boolean.toString(isClassAvailable("java.lang.ModuleLayer")), source);
Copy link
Contributor

Choose a reason for hiding this comment

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

*.feature.jpms is not used, so I would remove it -- no need to leave in the code unnecessary stuff.

@joakime
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-54…
2aa1a68 
…92-java-features-start-properties
@joakime
Issue #5492 - changed from review
f2bed13 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime mentioned this pull request Feb 18, 2021
Closed
@joakime joakime merged commit bbcae23 into jetty-9.4.x Feb 18, 2021
@joakime joakime deleted the jetty-9.4.x-5492-java-features-start-properties branch February 18, 2021 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw Awaiting requested review from gregw

Assignees

@joakime joakime

Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to manage start modules by java feature
4 participants
@joakime @janbartel @sbordet @gregw