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

Update apollo service in docker compose to wait for graphql service #3150

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

joshmeek
Copy link

@joshmeek joshmeek commented Aug 12, 2020

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • add a changelog entry in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

This PR makes use of server's post-start.sh to have the apollo service properly wait for the graphql service to be running and healthy.

This PR depends on PrefectHQ/server#41

Why is this PR important?

In its current form we will commonly see an error when running prefect server start that looks like:

apollo_1    |
apollo_1    | > @ serve /apollo
apollo_1    | > node dist/index.js
apollo_1    |
apollo_1    | Building schema...
apollo_1    | { FetchError: request to http://graphql:4201/graphql/ failed, reason: connect ECONNREFUSED 172.31.0.4:4201
apollo_1    |     at ClientRequest.<anonymous> (/apollo/node_modules/node-fetch/lib/index.js:1455:11)
apollo_1    |     at ClientRequest.emit (events.js:182:13)
apollo_1    |     at Socket.socketErrorListener (_http_client.js:392:9)
apollo_1    |     at Socket.emit (events.js:182:13)
apollo_1    |     at emitErrorNT (internal/streams/destroy.js:82:8)
apollo_1    |     at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
apollo_1    |     at process._tickCallback (internal/process/next_tick.js:63:19)
apollo_1    |   message:
apollo_1    |    'request to http://graphql:4201/graphql/ failed, reason: connect ECONNREFUSED 172.31.0.4:4201',
apollo_1    |   type: 'system',
apollo_1    |   errno: 'ECONNREFUSED',
apollo_1    |   code: 'ECONNREFUSED' }
apollo_1    | Trying again in 3 seconds...

This is due to docker-compose telling apollo it can start because the graphql container is up however the graphql service has not finished standing itself up—thus leading to this error. This error output can be confusing to new users.

@joshmeek joshmeek requested a review from jcrist as a code owner August 12, 2020 22:52
@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #3150 into master will not change coverage.
The diff coverage is n/a.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

This LGTM but let's hold off on merging until we can rebuild the server images and chat about server / core image compatibilities. Ultimately @jlowin and I landed on this proposal: #3149 but I want to have something like that in place prior to releasing this enhancement.

Copy link
Member

@cicdw cicdw 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 so much cleaner now! Nicely done

@joshmeek joshmeek merged commit 04961af into master Aug 24, 2020
@joshmeek joshmeek deleted the apollo_service_wait branch August 24, 2020 17:47
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