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 ClientNotActiveException instead of generic HazelcastException #13525
Throw ClientNotActiveException instead of generic HazelcastException #13525
Conversation
@@ -377,6 +378,9 @@ private Connection getConnection(Address target, boolean asOwner, boolean acquir | |||
} | |||
|
|||
private void checkAllowed(Address target, boolean asOwner, boolean acquiresResources) throws IOException { | |||
if (!alive) { |
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.
HazelcastClientNotActiveException
does not extend HazelcastException
. This is a behaviour change then. Is it possible some user was catching a HazelcastException?
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.
HazelcastException is retryable in client invocation system. When hazelcast exception is thrown from here, invocation system catches that throws as ClientNotActiveException to user.
https://github.com/hazelcast/hazelcast/blob/master/hazelcast-client/src/main/java/com/hazelcast/client/spi/impl/ClientInvocation.java#L209
But there is an unncessary wrapping there. It results as a ClientNotActiveException which has HazelcastException("client is shutting down") as cause.
There are a couple, usages of connection manager directly like txns which should handle HazelcastException themselves. In that case, we also convert it to ClientNotActiveException
So in almost all of the cases, user get ClientNotActiveException. Even if there were edge cases that user gets HazelcastException before, I would like to consider them as bug and fix it, rather then thinking this as behaviour change.
How does that sound ?
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.
I can admit I'm a bit lost here. It's your call. I'm just thinking if this could affect some user code like:
try {
// use hazelcast
} catch (HazelcastException expected) {
// all is good, I expected this!
}
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.
This is also my worry.
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.
Well as mentioned above, if we are not throwing ClientNotActiveException
where we supposed to throw it is a bug.
And depending on when the clients shutdown we could be throwing ClientNotActiveException
or HazelcastException
. That means, user should already prepared for ClientNotActiveException
. This fix is to correct one more place that we are throwing HazelcastException
instead of ClientNotActiveException
.
That is why, I see no problem with backward compatibility.
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.
Would it be possible (or cause any side affects) if IllegalStateException (base class of HazelcastClientNotActiveException) derive from HazelcastException instead of 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.
IllegalStateException can't derive from HazelcastException, because it is from java.lang library. It is not our class.
alive check in ClientConnectionManager is moved to a more centralized method and type of the exception is fixed. (cherry picked from commit e244fe7)
791ef2a
to
f67b9a2
Compare
alive check in ClientConnectionManager is moved to a more centralized
method and type of the exception is fixed.
(cherry picked from commit e244fe7)