jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bennyvw <...@git.apache.org>
Subject [GitHub] jmeter issue #325: 61544 - Added read, browse and clear as communication sty...
Date Fri, 10 Nov 2017 21:03:24 GMT
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 <notifications@github.com>:
    > 
    > 
    >     @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/
    
    
    benny@smaragd-it.nl mailto:benny@smaragd-it.nl



---

Mime
View raw message