-
Notifications
You must be signed in to change notification settings - Fork 21
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
[improvement] Do not throw error for Temporary Topic/Queue creation and deletion without admin privileges #154
Conversation
… only when Admin. Responsibility to create and delete will be delegated to the server.
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 have left a couple of comments in the code.
Then we must add a configuration flag to allow this lenient handling of temporary topic creation errors.
We must also add unit tests that cover both the 2 changes: with the new mode (expected no error) and with the standard mode (exepcting an error to be thrown)
pulsar-jms/src/main/java/com/datastax/oss/pulsar/jms/PulsarConnection.java
Show resolved
Hide resolved
pulsar-jms/src/main/java/com/datastax/oss/pulsar/jms/PulsarTemporaryDestination.java
Outdated
Show resolved
Hide resolved
…orary topic flow without admin privileges and changed exception handling to be specific.
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.
Very good. We are on our way
I have left only one comment about the test.
connection.start(); | ||
try (Session session = connection.createSession()) { | ||
if (expectAdminErrors) { | ||
assertThrows(IllegalStateException.class, session::createTemporaryTopic); |
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.
createTemporaryTopic
and other JMS Methods must never throw unchecked exceptions.
JMS 1.x methods should throw JMSException and JMS 2.x methods should throw RuntimeJMSException (or something like that)
I think that Utils.handleException already performs this conversion, I wonder why the test passes
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
is a checked Exception extendingJMSException
in JMS 2.x. -
Utils.handleException
is castingIllegalStateException
intoJMSException
and returning. assertThrows passes because it uses isInstance() method of Class. -
I am making it
JMSException
in the test for clarity,
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.
oh, thanks I thought it was java.lang.IllegalStateException
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.
LGTM
connection.start(); | ||
try (Session session = connection.createSession()) { | ||
if (expectAdminErrors) { | ||
assertThrows(IllegalStateException.class, session::createTemporaryTopic); |
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.
oh, thanks I thought it was java.lang.IllegalStateException
jms.usePulsarAdmin=false
, do not throw error for temporary topic/queue creation or deletion.autoTopicCreation
for non-partitioned topics and auto delete functionality for inactive topics.jms.allowTemporaryTopicWithoutAdmin
for making the changes configurable. Whentrue
, no errors will be thrown and temporary topics/queues wouldn't need admin access. Whenfalse
, errors will be thrown and temporary topic/queue creation is forbidden.