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

Only track released versions in oldVersions. #13096

Merged
merged 3 commits into from Feb 13, 2024

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Feb 12, 2024

This would avoid test failures when a release is in progress.

@jpountz jpountz requested a review from s1monw February 12, 2024 17:23
@jpountz
Copy link
Contributor Author

jpountz commented Feb 12, 2024

More fixes are needed, I'm looking.

@jpountz
Copy link
Contributor Author

jpountz commented Feb 12, 2024

We used to have logic to allow up to one missing file to account for in-progress releases: https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java#L805-L843. We need to figure out a way to replicate this logic with the new parameterized testing.

It would help if we could have access to an authoritative list of released versions, e.g. via the DOAP file maybe?

@uschindler
Copy link
Contributor

We used to have logic to allow up to one missing file to account for in-progress releases: https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java#L805-L843. We need to figure out a way to replicate this logic with the new parameterized testing.

Hahaha that logic is crazy....

It would help if we could have access to an authoritative list of released versions, e.g. via the DOAP file maybe?

This could work, but that would require network access at test time which is not allowed (security manager). Replicating the DOAP file to the repo seems like it defeats its purpose.

I tend to replicate the previous logic.

@uschindler
Copy link
Contributor

See this issue for more insights to the "crazy logic": https://issues.apache.org/jira/browse/LUCENE-5936

@uschindler
Copy link
Contributor

@RmuirSays: "yes please, lets not do the network. the smoketester can do that. In fact mike already wrote the code."

@jpountz
Copy link
Contributor Author

jpountz commented Feb 12, 2024

@uschindler I also have a bias against adding more dependencies on network resources, this is why I was thinking of the DOAP files since we have them in the repository: https://github.com/apache/lucene/blob/main/dev-tools/doap/lucene.rdf.

@uschindler
Copy link
Contributor

@uschindler I also have a bias against adding more dependencies on network resources, this is why I was thinking of the DOAP files since we have them in the repository: https://github.com/apache/lucene/blob/main/dev-tools/doap/lucene.rdf.

Oh that's new to me. Cool idea. Parsing with a simple stax parser should work. Unfortunately those are outside of test resources and can't be loaded easily.

I am tempted to add the legacy behaviour back. It's proven to work since 10 years. The code wasn't changed since that old issue.

@uschindler
Copy link
Contributor

uschindler commented Feb 12, 2024

P.S.: Unrelated to this issue here: we should update the doap file to use https links (not for the namespaces and controlled vocabularies, but those to our project homepage).

See #13097

@jpountz
Copy link
Contributor Author

jpountz commented Feb 13, 2024

I pushed a new commit that revives the old logic that allowed up to one version missing backward-compatibility data.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Thanks. We may look into better solutions at a later stage. For now this is perfect and stops builds from failing.

@uschindler
Copy link
Contributor

With our last change, we may need to change the script that adds the backwards indexes. Or is this now how it worked before?

@jpountz
Copy link
Contributor Author

jpountz commented Feb 13, 2024

@uschindler I'm not sure to understand what you mean. The script that adds bw compat indices indeed needs updating to update the oldVersions array when introducing bw compat indices for a new release.

@jpountz
Copy link
Contributor Author

jpountz commented Feb 13, 2024

(which is what Simon is doing at #13095)

@jpountz jpountz merged commit d7e0a7f into apache:main Feb 13, 2024
4 checks passed
@jpountz jpountz deleted the fix_old_unreleased_versions branch February 13, 2024 09:48
@uschindler
Copy link
Contributor

Thanks!

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.

None yet

3 participants