Skip to content

Strip pre-java-9 version check in Metrics as Velocity targets 21+#1836

Open
WouterGritter wants to merge 4 commits into
PaperMC:dev/3.0.0from
WouterGritter:simplify-java-version-check
Open

Strip pre-java-9 version check in Metrics as Velocity targets 21+#1836
WouterGritter wants to merge 4 commits into
PaperMC:dev/3.0.0from
WouterGritter:simplify-java-version-check

Conversation

@WouterGritter

Copy link
Copy Markdown
Member

As Velocity builds to target java 21, there's really no need to support legacy version parsing.

kennytv
kennytv previously requested changes Jul 3, 2026

@kennytv kennytv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Runtime.version() contains the feature version and extras

@WouterGritter

WouterGritter commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Runtime.version() contains the feature version and extras

Runtime.version().toString() will produce an output like 21.0.1+12-LTS, not the 21.0.1 that System.getProperty("java.version") provides.

I'd propose the following refactor:

metrics.addCustomChart(new DrilldownPie("java_version", () -> {
  Runtime.Version version = Runtime.version();

  return Map.of(
      "Java " + version.feature(),
      Map.of(System.getProperty("java.version"), 1));
}));

but it seems a bit odd to mix the old "api" (system property) and the new Runtime.Version API here. Thoughts?

Edit: I think it goes without saying that we shouldn't mess up the output to divide bStats metrics after this PR.

Edit 2: See latest commit, it's pretty easy to reconstruct the java.version system property exactly given a Runtime.Version

@WouterGritter WouterGritter requested a review from kennytv July 3, 2026 09:59
@kennytv

kennytv commented Jul 3, 2026

Copy link
Copy Markdown
Member

Mixing is fine I'd say

@WouterGritter

Copy link
Copy Markdown
Member Author

Mixing is fine I'd say

Better than the javaVersion helper method? I'm leaning more towards that approach (the current state of the PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants