Skip to content

docker: make home wait for solr via healthcheck in compose.override.yaml#12325

Merged
RayBB merged 1 commit intointernetarchive:masterfrom
modi02:scripts/home-wait-for-solr
Apr 9, 2026
Merged

docker: make home wait for solr via healthcheck in compose.override.yaml#12325
RayBB merged 1 commit intointernetarchive:masterfrom
modi02:scripts/home-wait-for-solr

Conversation

@modi02
Copy link
Copy Markdown
Contributor

@modi02 modi02 commented Apr 9, 2026

Closes #12321

Summary

Replaces the manual Solr polling loop in docker/ol-home-start.sh with
Docker's native healthcheck and depends_on mechanism.

Changes

Testing

Run docker compose up and verify the home service starts only after
Solr is healthy, without the manual polling loop.

Checklist

  • healthcheck uses /solr/openlibrary/admin/ping endpoint
  • home service depends_on solr with condition: service_healthy
  • Solr wait loop removed from ol-home-start.sh

Copilot AI review requested due to automatic review settings April 9, 2026 05:50
@github-project-automation github-project-automation Bot moved this to Waiting Review/Merge from Staff in Ray's Project Apr 9, 2026
@mekarpeles
Copy link
Copy Markdown
Member

Thank you for this contribution, @modi02!

@RayBB is assigned to this PR and currently has:

  • 11 open PR(s) of equal or higher priority to review first
PR triage checklist (maintainers / Pam)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — configuration/infrastructure change; no unit tests needed
  • Proof of testing — PR body includes a description of how to verify the change

Note

This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the local Docker dev workflow so the home service waits for Solr readiness via Docker Compose healthchecks, replacing the manual Solr polling loop previously embedded in docker/ol-home-start.sh.

Changes:

  • Removed the “Waiting for Solr…” polling loop from docker/ol-home-start.sh.
  • Added a Solr HTTP healthcheck in compose.override.yaml and made home depend on Solr being healthy before starting.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
docker/ol-home-start.sh Removes the in-container Solr wait loop so startup sequencing is handled by Compose.
compose.override.yaml Introduces Solr healthcheck + home depends_on gating on service_healthy.

Comment thread compose.override.yaml Outdated
Comment on lines +62 to +66
healthcheck:
test: ["CMD", "curl", "-sf", "http://localhost:8983/solr/openlibrary/admin/ping"]
interval: 5s
timeout: 5s
retries: 10
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

healthcheck is indented at the same level as the service definitions (e.g., solr-updater:), so it will be treated as a separate service named healthcheck rather than the solr service’s healthcheck. This will prevent depends_on: condition: service_healthy from working and may make the compose file invalid. Indent healthcheck: (and its fields) so it is under the solr: service block.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Please restore the new line at the end of the script file and use the health check that we added in the most recent pull request. You may need to branch off the most recent version of master.

https://github.com/internetarchive/openlibrary/pull/11570/changes

@RayBB
Copy link
Copy Markdown
Collaborator

RayBB commented Apr 9, 2026

Also, please include screenshots of what happens when you run Docker Compose up and preferably timings before and after.

@modi02 modi02 force-pushed the scripts/home-wait-for-solr branch 7 times, most recently from 09ad38a to 447dc33 Compare April 9, 2026 06:18
Copy link
Copy Markdown
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Almost there, just need to remove the submodule changes you committed.

Should be possible to do this with make git or follow our git guide on docs.openlibrary.org for more info.

@RayBB RayBB added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 9, 2026
@modi02 modi02 force-pushed the scripts/home-wait-for-solr branch from 447dc33 to f8323c0 Compare April 9, 2026 06:37
@github-actions github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 9, 2026
@modi02 modi02 force-pushed the scripts/home-wait-for-solr branch from f8323c0 to 3750143 Compare April 9, 2026 07:14
@modi02 modi02 force-pushed the scripts/home-wait-for-solr branch from 3750143 to c1df5e2 Compare April 9, 2026 07:21
Copy link
Copy Markdown
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

This is awesome, good job! It cuts startup time by about 24% in my case (8.501s → 6.473s)

@RayBB RayBB merged commit f1b7125 into internetarchive:master Apr 9, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from Waiting Review/Merge from Staff to Done in Ray's Project Apr 9, 2026
@modi02
Copy link
Copy Markdown
Contributor Author

modi02 commented Apr 9, 2026

Hi @RayBB, glad the improvement was useful! Apologies for the delayed comment, had some trouble attaching the screenshots earlier, but it looks like you were able to verify the fix directly.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

make home wait for solr in compose.override.yaml

4 participants