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

Arc - Make it possible to inspect shutdown reason from ShutdownEvent #26752

Merged
merged 1 commit into from
Jul 17, 2022

Conversation

manovotn
Copy link
Contributor

Fixes #16976

A simple way to expand ShutdownEvent so that users can inspect the cause of shutdown and differentiate between standard and non-standard.

@geoand is this at least close to what you meant? :)

@manovotn manovotn requested a review from geoand July 15, 2022 09:43
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/core labels Jul 15, 2022
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I had in mind!

@manovotn
Copy link
Contributor Author

Also CCing @Ladicek and @mkouba who might have something to add.

@Ladicek
Copy link
Contributor

Ladicek commented Jul 15, 2022

Makes sense to me, but I'd rather not expose the enum publicly, as it's hard to extend. Really the only thing the user can figure out is a boolean value, so in my opinion, we should expose a single boolean-returning method (called e.g. isStandardShutdown or so). If we later need to expose more detailed information (e.g. which signal triggered that shutdown, or the exit code maybe), that becomes a bit easier in my opinion. WDYT?

@manovotn
Copy link
Contributor Author

Really the only thing the user can figure out is a boolean value, so in my opinion, we should expose a single boolean-returning method

Yea, we could do that.

(e.g. which signal triggered that shutdown, or the exit code maybe)

From the code I don't think we can do this apart from native mode where we use SignalHandler and even then it is based on env property. Which is probably even more of a reason to just expose the boolean.

I'll adjust the PR in a while

@manovotn
Copy link
Contributor Author

@Ladicek updated. User can now only get to a boolean value derived from ShutdownReason.

I should also note that I tested these changes with quickstarts as there is no way to add automated test for it.

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

SHUTDOWN_REASON shouldn't really be upper-case, as it's not a constant, but that's just nit-picking :-)

@manovotn
Copy link
Contributor Author

SHUTDOWN_REASON shouldn't really be upper-case, as it's not a constant, but that's just nit-picking :-)

Damn, can't get that past you even on Friday :-D
Fixed now.

@manovotn manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 15, 2022
@quarkus-bot

This comment has been minimized.

@gsmet gsmet merged commit c5adb46 into quarkusio:main Jul 17, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 17, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Jul 17, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 17, 2022
@manovotn manovotn deleted the issue16976 branch July 25, 2022 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/core kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make shutdown cause/trigger available
5 participants