Skip to content

Conversation

@felixbarny
Copy link
Member

Consider the second commit optional. I can remove it if you don't like it.

@ghost
Copy link

ghost commented Jun 7, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-06-08T10:39:12.691+0000

  • Duration: 48 min 8 sec

Test stats 🧪

Test Results
Failed 0
Passed 2957
Skipped 22
Total 2979

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

I do like the parsing and testing touches, but I think relying on suffix for ordering is not solid.
While this is obvious:

Version.of(1.2.3-SNAPSHOT).equals(Version.of(1.2.3-RC0)) == false

This is just arbitrary:

Version.of(1.2.3-SNAPSHOT).compareTo(Version.of(1.2.3-RC0)) </> 0

The problem is that making those inconsistent could cause trouble specifically in sorted sets (see the Comparable javadoc).

Note that the parsing logic we have now in Version is not necessarily about identifying version prefixes and suffixes as you think about them, but a way to extract the version part out of a string, for example, such that comes from a jar manifest. The redhat httpclient example is real.

Bottom line, I think it's best to make both equality check and ordering rely only on the version numbers, but still maintain the hasSuffix and use it as you already did.

@felixbarny
Copy link
Member Author

The problem is that making those inconsistent could cause trouble specifically in sorted sets (see the Comparable javadoc).

I don't quite get this part as the sort is consistent with equals. While 1.2.3-SNAPSHOT > 1.2.3-RC0 may be arbitrary, it is consistent.

But yeah, overall it seems it doesn't quite work out due to the .redhat suffix thing.

@eyalkoren
Copy link
Contributor

I don't quite get this part as the sort is consistent with equals. While 1.2.3-SNAPSHOT > 1.2.3-RC0 may be arbitrary, it is consistent.

Sorry for not being clear enough. IMO, the best way to reflect versioning equality/comparison logic is:

Version.of(1.2.3-SNAPSHOT).equals(Version.of(1.2.3-RC0)) == false;
Version.of(1.2.3-SNAPSHOT).compareTo(Version.of(1.2.3-RC0)) == 0;

However, I think we should avoid that due to the inconsistency issue.

@felixbarny
Copy link
Member Author

Gotcha, I created a method that lets you remove suffixes. I think that's the best of both worlds. We can ignore suffixes in the element matcher where we know we have to deal with unorthodox .redhat versions.

@felixbarny felixbarny marked this pull request as ready for review June 8, 2022 09:27
@felixbarny felixbarny enabled auto-merge (squash) June 8, 2022 09:27
@github-actions
Copy link

github-actions bot commented Jun 8, 2022

/test

1 similar comment
@felixbarny
Copy link
Member Author

/test

@felixbarny felixbarny merged commit 6e634bf into elastic:main Jun 8, 2022
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.

2 participants