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

servicebus jms queue sample update #158

Conversation

jialigit
Copy link
Contributor

@jialigit jialigit commented Jan 19, 2022

  • Attention: remember to cherry pick to spring-cloud-azure_4.0.0-beta.4 branch

Purpose

*Update the ServiceBus Jms Queue sample

  1. Update configuration yml file.
  2. Update README file.
  3. Refactor code, including Filename, variable names...

Does this introduce a breaking change?

[ ] Yes
[x ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

@backwind1233
Copy link
Contributor

@yiliuTo please help review this PR.

@backwind1233 backwind1233 added the azure-spring All azure-spring related issues label Jan 19, 2022
@JmsListener(destination = QUEUE_NAME, containerFactory = "jmsListenerContainerFactory")
/**
* Message consumer
* @param user user
Copy link
Contributor

Choose a reason for hiding this comment

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

We should give more detailed explanation for what the parameter of User is used as.

import org.springframework.jms.core.JmsTemplate;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

/**
* Message Producer Mannually.
*/
@RestController
public class QueueSendController {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's unify the class naming and remove Controller suffix.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Controller can be remained I think, it has a post request.

@Autowired
private JmsTemplate jmsTemplate;

/**
* @param message username
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add the introduction of the method, also we could give full explanation of what the paremeter of message is used as

import javax.jms.Session;

/**
* For reqeust-response pattern of Jms.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here

private JmsTemplate jmsTemplate;

@Autowired
private JmsMessagingTemplate jmsMessagingTemplate;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this used for?

* Premium Tier Only
*/
@Component
@Profile("sync")
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we call this profile as sync? Given the description of class javadoc, this profile of sample is trying to introduce the use case of "send and receive" of JmsMessagingTemplate, but do we have special support of it in our library?

*/
@RestController
@Profile("sync")
public class SyncQueueSendController {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

.createSession(false, Session.AUTO_ACKNOWLEDGE);
ObjectMessage objectMessage = session.createObjectMessage(new User(message));
objectMessage.setJMSCorrelationID(UUID.randomUUID().toString());
objectMessage.setJMSReplyTo(connectionFactory.createContext().createTemporaryQueue());
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the temporary queue here mean? will it create a queue in Service Bus? And we can comment here to introduce the meaning of message properties we configured.

objectMessage.setJMSDeliveryMode(DeliveryMode.NON_PERSISTENT);

return jmsMessagingTemplate.convertSendAndReceive(new JmsQueue(queueName),
objectMessage, User.class); //this operation seems to be blocking + sync
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't have "seems to be" here

pricing-tier: ${PRICING_TIER}
pool:
enabled: true # To be true to enable the JmsPoolConnectionFactory bean. Default false.
queuename: que001
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow unified property naming schema and use kebab here

@yiliuTo
Copy link
Contributor

yiliuTo commented Jan 20, 2022

We can update this sample and introduce the usage of caching and pooled connection factory, and update the sample of topic as well

In your terminal, run `mvn clean spring-boot:run`.


Navigate to the project root directory, then run the maven command:
Copy link
Contributor

Choose a reason for hiding this comment

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

we can update the template and then update all the descriptions here.
issue link

please revert it here.

```shell
mvn clean spring-boot:run
```

## Verify This Sample

1. Verify in your app's logs that a similar message was posted:
Copy link
Contributor

Choose a reason for hiding this comment

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

similar messages were posted

```
curl -d "" http://localhost:8080/queue?message=hello
```

2. Verify in your app's logs that a similar message was posted:
3. [Optional] Verify in your app's logs that a similar message was posted:
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@jialigit
Copy link
Contributor Author

I should propose different pr for different issues. So this closed for now.

@jialigit jialigit closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-spring All azure-spring related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants