Adding term catch to load job cache script #2382
Conversation
| else | ||
| echo "Second SIGTERM received, dipping out" | ||
| rm -r -f "$tmpdir" | ||
| exit $? |
There was a problem hiding this comment.
This is following the pattern that was established before you but this doesn't look right to me. I think if we Ctrl+C this script, we wouldn't want to return a successful exit code (0). But this is returning the exit code of the rm command above it, so if the rm works, then we return success.
Thoughts?
There was a problem hiding this comment.
Hm, maybe instead we exit with a 1 here instead? I think that might be prudent.
There was a problem hiding this comment.
Works for me. Can you also add it to the KILL INT trap too?
warehouse/ingest-scripts/src/main/resources/bin/ingest/load-job-cache.sh
Outdated
Show resolved
Hide resolved
| # prepare a directory with links to all of the files/directories to put into the jobcache | ||
| tmpdir=`mktemp -d $MKTEMP_OPTS` | ||
| trap 'rm -r -f "$tmpdir"; exit $?' INT TERM EXIT | ||
| trap 'rm -r -f "$tmpdir"; exit 1' INT EXIT |
There was a problem hiding this comment.
Doesn't
trap 'blah ; exit 1' EXIT
guarantee that the script will always exit w/ non-zero status?
There was a problem hiding this comment.
going off a discussion with Jeff it looks like if TERM/KILL signals are being sent while this script is running it probably makes sense to return with a non-zero status to imply the script did not exit clean. If the trap isn't encountered though, we would want to exit with a 0 in that case.
There was a problem hiding this comment.
Trap arg for EXIT will fire even on normal exit of the script. So, I don't see how we'll ever get a zero exit status
E.g., if you run the script below, you'll get non-zero return status:
#!/bin/bash
trap 'echo trapping; exit 1' INT EXIT
exit 0
There was a problem hiding this comment.
Ah ok, now I follow. I'll see if I can extract out the EXIT signal from that logic
No description provided.