Github user bennyvw commented on the issue:
https://github.com/apache/jmeter/pull/325
Philippe,
Thanks for your quick review. I am going to pay attention to your remarks
asap.
Regards,
Benny.
> Op 10 november 2017 om 21:20 schreef Philippe M
<[email protected]>:
>
>
> @pmouawad requested changes on this pull request.
>
> Thanks a lot for this very interesting contribution.
> Find my remarks below.
> I'll be happy to merge this PR once you can take into account those
remarks and possible remarks from other members.
> Regards
>
>
> ---------------------------------------------
>
> In docs/usermanual/component_reference.html
https://github.com/apache/jmeter/pull/325#discussion_r150327021 :
>
> > @@ -3813,7 +3813,7 @@ <h3 id="JMS_Publisher_parms1">
>
>
> HTML files are generated from XML. Could you update
comonent_reference.xml instead ?
>
>
> ---------------------------------------------
>
> In docs/usermanual/component_reference.html
https://github.com/apache/jmeter/pull/325#discussion_r150327166 :
>
> > + queue referenced by <span
class="code">message.getJMSReplyTo()</span>.
> +</dd>
> +
> +<dt>
> +<span class="code">Read</span>
> +</dt>
> +<dd> will read a message from an outgoing queue which has no
listeners attached. This can be convenient for testing purposes.
> + This method can be used if you need to handle queues without a
binding file (in case the jmeter-jms-skip-jndi library is used),
> + which only works with the JMS Point-to-Point sampler.
> + In case binding files are used, one can also use the JMS
Subscriber Sampler for reading from a queue.
> +</dd>
> +
> +<dt>
> +<span class="code">Browse</span>
> +</dt>
> +<dd> will determine the current queue depth without removing
messages from the queue, returning the number of messages on the queue.
>
> Would it be possible to provide screenshots as it makes documentation
much clearer
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150327668 :
>
> > res.setResponseMessage(e.getLocalizedMessage());
> }
> }
> res.sampleEnd();
> return res;
> }
>
> + private void handleBrowse(SampleResult res) throws JMSException {
> + LOGGER.debug("isBrowseOnly");
> + StringBuffer sb = new StringBuffer("");
>
> StringBuilder would be better here, and wherever StringBuffer is used
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328109 :
>
> > + Integer.parseInt(getPriority()),
Long.parseLong(getExpiration()));
> + res.setRequestHeaders(Utils.messageProperties(msg));
> + if (replyMsg == null) {
> + res.setResponseMessage("No reply message received");
> + } else {
> + if (replyMsg instanceof TextMessage) {
> + res.setResponseData(((TextMessage)
replyMsg).getText(), null);
> + } else {
> + res.setResponseData(replyMsg.toString(), null);
> + }
> +
res.setResponseHeaders(Utils.messageProperties(replyMsg));
> + res.setResponseOK();
> + }
> + }
> +
> + private String browseQueueForConsumption(Queue queue, String
jmsSelector, SampleResult res, StringBuilder buffer,
>
> Isn't name confusing here ? AFAIU we are consuming here not browsing
right ?
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328248 :
>
> > + } else {
> + res.setResponseData(replyMsg.toString(), null);
> + }
> +
res.setResponseHeaders(Utils.messageProperties(replyMsg));
> + res.setResponseOK();
> + }
> + }
> +
> + private String browseQueueForConsumption(Queue queue, String
jmsSelector, SampleResult res, StringBuilder buffer,
> + StringBuilder propBuffer) {
> + String retVal = null;
> + try {
> + QueueReceiver consumer = session.createReceiver(queue,
jmsSelector);
> + Message reply =
consumer.receive(Long.valueOf(getTimeout()));
> + LOGGER.debug("Message: " + reply);
> + consumer.close();
>
> You should use try with resource pattern to ensure close happens ? Or
use try/finally and closeQuietly depending on what you want to do
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328288 :
>
> > + try {
> + QueueReceiver consumer = session.createReceiver(queue,
jmsSelector);
> + Message reply =
consumer.receive(Long.valueOf(getTimeout()));
> + LOGGER.debug("Message: " + reply);
> + consumer.close();
> + if (reply != null) {
> + res.setResponseMessage("1 message(s) received
successfully");
> + res.setResponseHeaders(reply.toString());
> + TextMessage msg = (TextMessage) reply;
> + retVal = msg.getText();
> + extractContent(buffer, propBuffer, msg);
> + } else {
> + res.setResponseMessage("No message received");
> + }
> + } catch (Exception ex) {
> + ex.printStackTrace();
>
> This must be removed
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328360 :
>
> > + QueueReceiver consumer = session.createReceiver(queue,
jmsSelector);
> + Message reply =
consumer.receive(Long.valueOf(getTimeout()));
> + LOGGER.debug("Message: " + reply);
> + consumer.close();
> + if (reply != null) {
> + res.setResponseMessage("1 message(s) received
successfully");
> + res.setResponseHeaders(reply.toString());
> + TextMessage msg = (TextMessage) reply;
> + retVal = msg.getText();
> + extractContent(buffer, propBuffer, msg);
> + } else {
> + res.setResponseMessage("No message received");
> + }
> + } catch (Exception ex) {
> + ex.printStackTrace();
> + LOGGER.error(ex.getMessage());
>
> Any logging requires some contextual information otherwise it's
useless.
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328441 :
>
> > + if (corrID == null) {
> + corrID = message.getJMSMessageID();
> + messageBodies = messageBodies + numMsgs + " -
MessageID: " + corrID + ": " + message.getText()
> + + "\n";
> + } else {
> + messageBodies = messageBodies + numMsgs + " -
CorrelationID: " + corrID + ": " + message.getText()
> + + "\n";
> + }
> + numMsgs++;
> + }
> + res.setResponseMessage(numMsgs + " messages available on
the queue");
> + res.setResponseHeaders(qBrowser.toString());
> + return (messageBodies + queue.getQueueName() + " has " +
numMsgs + " messages");
> + } catch (Exception e) {
> + res.setResponseMessage("Error counting message on the
queue");
> + e.printStackTrace();
>
> Same remarks as above
>
>
> ---------------------------------------------
>
> In
src/protocol/jms/org/apache/jmeter/protocol/jms/sampler/JMSSampler.java
https://github.com/apache/jmeter/pull/325#discussion_r150328999 :
>
> > + LOGGER.error(e.getMessage());
> + return "";
> + }
> + }
> +
> + private String clearQueue(Queue queue, SampleResult res) {
> + String retVal = null;
> + try {
> + QueueReceiver consumer = session.createReceiver(queue);
> + Message deletedMsg = null;
> + long deletedMsgCount = 0;
> + do {
> + deletedMsg = consumer.receiveNoWait();
> + if (deletedMsg != null) {
> + deletedMsgCount++;
> + deletedMsg.acknowledge();
>
> Shouldn't this be parameterized ? There are 4 modes, if we ACK for
AUTO_ACK that would be wrong no ?
>
> â
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
https://github.com/apache/jmeter/pull/325#pullrequestreview-75868433 , or mute
the thread
https://github.com/notifications/unsubscribe-auth/Aem4Xh4JYZs7qs0fAg01NRZYm8aTa2g-ks5s1LAlgaJpZM4QZnSl
.
>
>
>
Met vriendelijke groet,
Benny van Wijngaarden.
Tel. 0629556587http://www.smaragd-it.nl/
[email protected] mailto:[email protected]
---
