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

[improvement] Do not throw error for Temporary Topic/Queue creation and deletion without admin privileges #154

Merged
merged 7 commits into from
Nov 4, 2024

Conversation

sandeep-ctds
Copy link
Contributor

@sandeep-ctds sandeep-ctds commented Nov 4, 2024

  • Parent Ticket is STREAM-503
  • When jms.usePulsarAdmin=false, do not throw error for temporary topic/queue creation or deletion.
  • Relying on pulsar autoTopicCreation for non-partitioned topics and auto delete functionality for inactive topics.
  • Added jms.allowTemporaryTopicWithoutAdmin for making the changes configurable. When true, no errors will be thrown and temporary topics/queues wouldn't need admin access. When false, errors will be thrown and temporary topic/queue creation is forbidden.

… only when Admin.

Responsibility to create and delete will be delegated to the server.
Copy link
Collaborator

@eolivelli eolivelli left a 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)

Copy link
Collaborator

@eolivelli eolivelli left a 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);
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eolivelli

  • IllegalStateException is a checked Exception extending JMSException in JMS 2.x.

  • Utils.handleException is casting IllegalStateException into JMSException and returning. assertThrows passes because it uses isInstance() method of Class.

  • I am making it JMSException in the test for clarity,

Copy link
Collaborator

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

Copy link
Collaborator

@eolivelli eolivelli left a 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);
Copy link
Collaborator

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

@sandeep-ctds sandeep-ctds merged commit 0664b59 into master Nov 4, 2024
3 checks passed
@sandeep-ctds sandeep-ctds deleted the topic-creation-without-admin branch November 4, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants