HIVE-27295: Improve docker logging in AbstractExternalDB #4268
Conversation
| throw new RuntimeException("Container failed to be ready in " + MAX_STARTUP_WAIT/1000 + | ||
| " seconds"); | ||
| printDockerEvents(); | ||
| throw new RuntimeException("Container failed to be ready in " + MAX_STARTUP_WAIT/1000 + |
There was a problem hiding this comment.
Bit odd exception message. Minor stuff I know.
Container initialization is failed within x seconds. Please check the hive logs.
There was a problem hiding this comment.
yeah, odd, I'll change it
| try { | ||
| runCmdAndPrintStreams(new String[] { "docker", "events", "--since", "24h", "--until", "0s" }, 3); | ||
| } catch (Exception e) { | ||
| LOG.info("Error while getting docker events, this was an analytical best effort step, no further actions...", |
There was a problem hiding this comment.
Alternative text: There was a problem encountered while attempting to retrieve Docker events. The system made an analytical best effort to resolve the issue, and at this point, no further actions are necessary.
There was a problem hiding this comment.
ack, I'll edit this one too
your message makes sense, except that "to resolve the issue" is not 100% true, docker events cmd just lists the docker events to make us able to guess what happened, so this would be the ideal one:
A problem was encountered while attempting to retrieve Docker events (the system made an analytical best effort to list the events to reveal the root cause). No further actions are necessary.
| try { | ||
| runCmdAndPrintStreams(new String[] { "docker", "events", "--since", "24h", "--until", "0s" }, 3); | ||
| } catch (Exception e) { | ||
| LOG.info("Error while getting docker events, this was an analytical best effort step, no further actions...", |
There was a problem hiding this comment.
It should be warning. I guess it is more then an info, but not as important as an error. In an info message an "Error" could be confusing.
kgyrtkirk
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
LGTM. Since AbstractExternalDB is more or less a copy (unfortunately :( ) of DatabaseRule we may want to apply the same improvements there; it can be done as part of this PR or a separate one.
| } | ||
| } | ||
|
|
||
| public void printDockerEvents() { |
There was a problem hiding this comment.
absolutely not, I'll change,thanks
makes sense, I'm porting changes to DatabaseRule now, otherwise I'll forget forever |
kgyrtkirk
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
Kudos, SonarCloud Quality Gate passed!
|
…Rule (apache#4268) (Laszlo Bodor reviewed by Stamatis Zampetakis)
…Rule (apache#4268) (Laszlo Bodor reviewed by Stamatis Zampetakis)
4 participants
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?