Await for backend to be ready before showing link #1172
Conversation
GSK-1291 Waiting for backend to start message
Only print in CLI that server is started when all components are ready |
| proxy_read_timeout 360000s; | ||
| } | ||
|
|
||
| location /management { |
There was a problem hiding this comment.
good job using the existing API 👍
| return False | ||
|
|
||
|
|
||
| def _await_backend_ready(port: int): |
There was a problem hiding this comment.
nitpick: await has an asynchronous connotation in python (async/await). There's also an async sleep counterpart that'd be used for it: await asyncio.sleep()
In order to remove the confusion I'd rename it to just _wait_backend_ready
| def _await_backend_ready(port: int): | |
| def _wait_backend_ready(port: int): |
| if response.status_code != 200: | ||
| return False | ||
|
|
||
| return "UP" == response.json()['components']['readinessState']['status'] |
There was a problem hiding this comment.
I'm testing on dev setup, but my json structure isn't the same, I don't have components readinessState
There was a problem hiding this comment.
I got a different format, will use status instead to be sure.
| return False | ||
|
|
||
| return "UP" == response.json()['components']['readinessState']['status'] | ||
| except OSError: |
There was a problem hiding this comment.
requests can throw other errors (ConnectTimeout for example), I think we can simply catch BaseException and return False if it happens. (make sure to handle KeyboardInterrupt separately and reraise click.Abort())
In this case you can even replace
if response.status_code != 200:
return False
with
response.raise_for_status()
since it'll be caught by the same BaseException handler
| return None | ||
|
|
||
|
|
||
| def _backend_ready(endpoint) -> bool: |
There was a problem hiding this comment.
_is_backend_ready is a bit cleaner IMO
| backoff_time = 2 | ||
| up = False | ||
|
|
||
| while not up: |
There was a problem hiding this comment.
I think we should have a waiting limit. Let's say we wait for 3 minutes and if _backend_ready still returns False we break the loop and print an explanation like:
Giskard backend takes unusually long time to start, please check the logs with `giskard server logs backend`
Also I think it's worth adding a feedback while waiting, for example printing dots every backoff_time seconds, like this it's clear that the process isn't frozen, but is actually checking the status
click.echo(".", nl=False)
should do the trick, because print(end='') won't work in the loop without explicit stdout flushing
| else: | ||
| logger.warning( | ||
| "Giskard backend takes unusually long time to start, please check the logs with `giskard server " | ||
| "logs backend" |
There was a problem hiding this comment.
looks like a missing closing ` after "backend"
| "logs backend" | |
| "logs backend`" |
There was a problem hiding this comment.
or perhaps to make it even more readable you can break the long line before the giskard server logs backend command, for example after the first comma
| else: | ||
| logger.warning( | ||
| "Giskard backend takes unusually long time to start, please check the logs with `giskard server " | ||
| "logs backend" |
There was a problem hiding this comment.
or perhaps to make it even more readable you can break the long line before the giskard server logs backend command, for example after the first comma
Co-authored-by: Andrey Avtomonov <[email protected]>
Co-authored-by: Andrey Avtomonov <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
Hartorn
deleted the
feature/gsk-1291-waiting-for-backend-to-start-message
branch
Description
Await for backend to be ready before showing link
Type of Change