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

Default to exit on failure #417

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

zimmermatt
Copy link
Contributor

Upon discussion, though this seems simple on the surface, there are a
number of nuances that make this tricky to get right. For instance,
if a dependency has a non-daemon thread and the failure occurs in such a
way that a service such as discovery sees the app as in service, the app
could take traffic when it shouldn't.

The main change that occurred since this was put in place was a switch
to primarily use async loggers in the Insight Engineering apps. It was
previously observed that synchronous loggers would emit their logs, even
with the call to System.exit(). Ideally, we can restore that behavior
with async loggers, but that may also prove to not be easy.

Upon discussion, though this seems simple on the surface, there are a
number of nuances that make this tricky to get right. For instance,
if a dependency has a non-daemon thread and the failure occurs in such a
way that a service such as discovery sees the app as in service, the app
could take traffic when it shouldn't.

The main change that occurred since this was put in place was a switch
to primarily use async loggers in the Insight Engineering apps. It was
previously observed that synchronous loggers would emit their logs, even
with the call to `System.exit()`. Ideally, we can restore that behavior
with async loggers, but that may also prove to not be easy.
@zimmermatt zimmermatt merged commit bc7ea23 into Netflix:master Aug 5, 2019
@zimmermatt zimmermatt deleted the default-exit-on-failure branch August 5, 2019 18:20
@brharrington brharrington added this to the 2.1.1 milestone Aug 15, 2019
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