-
Notifications
You must be signed in to change notification settings - Fork 0
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
throw DeadlineExpiredException when deadline expiration is realized #16
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
public boolean isInternal() { | ||
return internal; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could alternatively handle this as two nested subclasses, like QosException. Not entirely clear which option is best in this case, but we have some time to think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i considered that, i don't have strong opinions either way. I do tend to prefer encoding something like this as a type, rather than a field.
b25cc24
to
f77d4c2
Compare
/** | ||
* Indicates that a deadline has expired. | ||
*/ | ||
public abstract class DeadlineExpiredException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should split this class out into a separate PR so that we can publish it in the API for this library and allow consumers to adopt logic that handles these exception types before we merge a change to actually throw the exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
can we make this a sealed type? We should also implement SafeLoggable imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there much utility in having a base type at all here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #25
cb0248c
to
1bfe002
Compare
2756993
to
6a42e2e
Compare
1bfe002
to
0466b1c
Compare
Generate changelog in
|
No description provided.