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

Solr 8.11.2 update #244

Merged
merged 8 commits into from
Nov 3, 2022
Merged

Solr 8.11.2 update #244

merged 8 commits into from
Nov 3, 2022

Conversation

DonRichards
Copy link
Member

This is untested.

I think this would work but I'm still testing it.

Copy link
Contributor

@nigelgbanks nigelgbanks left a comment

Choose a reason for hiding this comment

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

I have a few questions that might affect the upcoming release, and would require us to include some more info in the release notes, or do a minor/major bump.

  1. Are any changes required on the Drupal side after this version bump?
  2. Will this have an effect on someone's existing index (e.g. their solr-data Docker volume) if they were to switch images.
  3. Would they have to rebuild the index if it does?

Thanks!

solr/Dockerfile Outdated
@@ -3,11 +3,13 @@ ARG repository=local
ARG tag=latest
FROM --platform=$BUILDPLATFORM ${repository}/download:${tag} AS download

ENV SOLR_VERSION="8.11.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Using ENV adds layers to the image and shouldn't be added here. As the environment variables are used for runtime information, not build time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions for a replacement? Should I switch it to ARG?

Copy link
Contributor

Choose a reason for hiding this comment

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

ARG seems fine, we can change the other images for consistency as part of another issue (I'll create one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, great. Thanks for adding the ticket. Changes made.

solr/README.md Show resolved Hide resolved
solr/Dockerfile Outdated Show resolved Hide resolved
solr/README.md Outdated Show resolved Hide resolved
solr/README.md Show resolved Hide resolved
@DonRichards
Copy link
Member Author

@nigelgbanks To answer your original questions

  1. Are any changes required on the Drupal side after this version bump?

Not sure. It doesn't work for me yet so I imagine it might.

  1. Will this have an effect on someone's existing index (e.g. their solr-data Docker volume) if they were to switch images.

I'm glad you asked. I just looked it up here and yes there is. Not sure how to integrate this into the codebase

  1. Would they have to rebuild the index if it does?

They suggest always reindexing after an upgrade, I would assume a rebuild would be needed.

With all of that said, I'm not sure what the next steps should be for this PR.

@nigelgbanks
Copy link
Contributor

With all of that said, I'm not sure what the next steps should be for this PR.

At the least, we'll have to include it in the release notes. For isle-dc I'm not certain, there is a good solution as people can / are encouraged to maintain the .env file outside of source control, and we shouldn't be triggering re-builds of indexes if it's not appropriate.

@DonRichards
Copy link
Member Author

Sorry, I just realized I was clicking "Resolved" on your comments as though they were something for me to "resolve". lol

@nigelgbanks
Copy link
Contributor

Sorry, I just realized I was clicking "Resolved" on your comments as though they were something for me to "resolve". lol

Haha no worries, after you have pushed the fix you can mark them as resolved.

@DonRichards
Copy link
Member Author

OK, I think I made all of the suggested changes. But to recap, on the release we should comment on how there is documentation from Solr on how to upgrade the index when upgrading Solr from 7 to 8. We should give an example command that the community has tested to upgrade Solr's index like the example they gave but adjusted for isle-dc

./bin/solr start -c -Dsolr.http1=true -z localhost:2481/solr -s /path/to/solr/home

They also had one last comment that we should probably spell out how to do it manually

When all nodes have been upgraded to 8.0, restart each one without the -Dsolr.http1 parameter.

@DonRichards DonRichards changed the title Draft - Solr 8.11.2 update Solr 8.11.2 update Sep 7, 2022
@DonRichards
Copy link
Member Author

@nigelgbanks Do you see what's making the tests fail? I don't see what I need to change to get it to pass.

@nigelgbanks
Copy link
Contributor

nigelgbanks commented Sep 12, 2022

Looks to be the same ps issue:

BusyBox v1.34.1 (2021-11-23 00:57:35 UTC) multi-call binary.

Usage: ps [-o COL1,COL2=HEADER] [-T]

Show list of processes

        -o COL1,COL2=HEADER     Select columns for display
        -T                      Show threads
This script relies on a version of ps that supports the -p flag.

Please install a POSIX compliant version and try again.

Seems that https://issues.apache.org/jira/browse/SOLR-16191 is wrong is stating that it's fixed for 8.11.2.

@nigelgbanks
Copy link
Contributor

Looks like I read the ticket wrong, on reviewing their actual changes made for 8.11.2 it seems like they now have a hard requirement on using GNU version of ps rather than supporting both.

@nigelgbanks nigelgbanks marked this pull request as draft September 12, 2022 20:16
@nigelgbanks
Copy link
Contributor

@DonRichards I've changed this to a draft just to prevent it from going in before we cut a minor release (which should be pretty soon). Subsequently, it should be good to go into the next major release, for which we'll document the need for folks to rebuild their solr index.

@DonRichards
Copy link
Member Author

@nigelgbanks are you good with this dropping the "draft" status now?

@nigelgbanks
Copy link
Contributor

@DonRichards whoops yea we'll start working on the 2.0.0 release.

@nigelgbanks nigelgbanks marked this pull request as ready for review November 3, 2022 21:24
@nigelgbanks nigelgbanks mentioned this pull request Nov 3, 2022
@nigelgbanks
Copy link
Contributor

nigelgbanks commented Nov 3, 2022

@DonRichards I've raised a ticket, so I don't forget about updating the release notes. #261

@nigelgbanks nigelgbanks merged commit 219677c into main Nov 3, 2022
@nigelgbanks nigelgbanks deleted the solr-8_11_2-update branch November 3, 2022 21:27
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