Skip to content

Use identifiable exit code for application configuration errors#16049

Merged
electrum merged 1 commit intotrinodb:masterfrom
erichwang:exitcode
Feb 9, 2023
Merged

Use identifiable exit code for application configuration errors#16049
electrum merged 1 commit intotrinodb:masterfrom
erichwang:exitcode

Conversation

@erichwang
Copy link
Contributor

Description

Configuration errors are always fatal and non-recoverable, so it helps to have a unique exit code.

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Configuration errors are always fatal and non-recoverable, so it helps
to have a unique exit code.
@cla-bot cla-bot bot added the cla-signed label Feb 9, 2023
@erichwang erichwang requested review from dain and electrum February 9, 2023 18:56
@electrum electrum merged commit 416d2c7 into trinodb:master Feb 9, 2023
@github-actions github-actions bot added this to the 407 milestone Feb 9, 2023
@erichwang erichwang deleted the exitcode branch February 9, 2023 20:49
@colebow
Copy link
Member

colebow commented Feb 13, 2023

Just as a friendly heads up, we generally skip release notes for improved error messaging because it's self-documenting when users encounter the errors.

@erichwang
Copy link
Contributor Author

@colebow thanks for weighing in. In this particular case, we might want a release note since the behavior of the process is changing in that it can return a different code now. I wouldn't say that is self-documenting since there isn't really a formal standard definition for return codes.

@colebow
Copy link
Member

colebow commented Feb 16, 2023

My point is more that users don't need to know about this change to benefit from it - when they bump into the new error code for the first time, they'll see the change organically. And if and only if they encounter the new error code themselves, this change won't impact how they use Trino.

If we want to document what error codes mean, then that should be via actual documentation, as searching through old release notes isn't a preferred way to find or track down information.

@erichwang
Copy link
Contributor Author

I see, btw we are superceding this PR with #16133

@hashhar
Copy link
Member

hashhar commented Feb 16, 2023

My point is more that users don't need to know about this change to benefit from it - when they bump into the new error code for the first time, they'll see the change organically. And if and only if they encounter the new error code themselves, this change won't impact how they use Trino.

Not really true. Anyone who wrote their own systemd unit or have configured stop hooks on k8s needs to know the change otherwise their hooks and units won't work as expected.

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

Development

Successfully merging this pull request may close these issues.

4 participants