ARTEMIS-1895 - Add duplicate metadata failure callback to ActiveMQServerPlugin #2116
Conversation
| @@ -263,6 +264,8 @@ private void validateClientID(ClientSession validateSession, String clientID) th | |||
| } catch (ActiveMQException e) { | |||
| if (e.getType() == ActiveMQExceptionType.DUPLICATE_METADATA) { | |||
| throw new InvalidClientIDException("clientID=" + clientID + " was already set into another connection"); | |||
| } else { | |||
| throw new InvalidClientIDException("Error setting clientID=" + clientID + ": " + e.getMessage()); | |||
There was a problem hiding this comment.
The original exception here may not be related to InvalidClientId, seems a bit dangerous to catchall and throw like this. Maybe bubble up or convert the exception, at least link the exception and add the intial cause.
|
@michaelandrepearce - good point, i fixed it to bubble up the ActiveMQException so it is handled properly by the client |
| con.setClientID("valid"); | ||
| con2.setClientID("valid"); | ||
| fail("Should have failed for duplicate clientId"); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Should this test that explicit exception type is thrown not just any old exception. As probably should fail if an unexpectes exception is ever thrown.
There was a problem hiding this comment.
I switched it to catch the InvalidClientIdException that gets thrown
There was a problem hiding this comment.
I also added another test to verify that ActiveMQException bubbles up and is converted to a JMSException by the client
| fail("Should have failed for duplicate clientId"); | ||
| } catch (InvalidClientIDException e) { | ||
| assertEquals(1, duplicateCount.get()); | ||
| throw e; |
There was a problem hiding this comment.
Problem here is assert will be bypassed and test will still pass if another exception thrown as tests expects JMSException.
Expects should be explicit and need a catch all with and assert fail
|
I removed the re-throw and the expect for the JMSException so it will only pass on InvalidClientIDException as that is the only type that should be thrown |
|
Looks good to me |
|
@clebertsuconic - is this ok to merge? |
|
I don't think it's related as the test it failed on doesn't have a plugin registered. I tried to see if I could trigger it to re-run the build in travis ci but i didn't see a way to do that. |
|
The easiest way to rebuild is to. |
…verPlugin Add a callback on duplicate metadata which will allow extra functionality to be added.
|
The build looks good now |
|
thanks a lot.. merging |
Add a callback on duplicate metadata which will allow extra functionality to be added.