[GitHub] activemq-artemis pull request #2490: V2 196

classic Classic list List threaded Threaded
74 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
GitHub user michaelandrepearce opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2490

    V2 196

    @franz1981 an alternative so we don't have to have a copy of CopyOnWriteArrayList, it does mean on add or remove consumer we have to invoke toArray which causes a copy, but this is not on hot path, so i think we should be good, and avoids us having to clone a jvm class.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/michaelandrepearce/activemq-artemis V2-196

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2490.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2490
   
----
commit d731ffe7288cb857fef1b97deff4b7dc18aeb6d7
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-12-31T13:22:02Z

    ARTEMIS-196 Implement Consumer Priority
   
    Add consumer priority support
    Includes refactor of consumer iterating in QueueImpl to its own logical class, to be able to implement.
    Add OpenWire JMS Test - taken from ActiveMQ5
    Add Core JMS Test
    Add AMQP Test
    Add Docs

commit b0c775840fc98b5d3f5f3485802de3270c614d9a
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2019-01-05T09:48:24Z

    Extract

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2490: V2 196

asfgit
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2490
 
    @michaelandrepearce Nice! Will take a look today or max tomorrow :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2490
 
    @franz1981 just ignore class comments, theyre the originals still, ill need to change, but wanted to get to you quickly so you have chance to look over. If you think this is better ill make final tidyup bits, such as class comments and replace the real PR's branch.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2490
 
    @franz1981 did you get a chance to look, do you think this is better than original solution?
   
    Am keen to get this feature into the next release cut.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2490: V2 196

asfgit
In reply to this post by asfgit
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2490
 
    @mochaelandrepearce sadly haven't had much time today to look into it :(
    Tomorrow I have already scheduled some time in the morning to take a look into this


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2490
 
    @franz1981 great thanks a million :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245955337
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java ---
    @@ -104,6 +119,11 @@ public void decodeRest(final ActiveMQBuffer buffer) {
           filterString = buffer.readNullableSimpleString();
           browseOnly = buffer.readBoolean();
           requiresResponse = buffer.readBoolean();
    +      if (buffer.readableBytes() > 0) {
    --- End diff --
   
    I assume this is to allow for old clients that don't send this value. Would a more specific version check be clearer here for later reference? Related, I'm guessing other changes already made for 2.7.0 have updated the version info since it doesn't look to change here?
   
    Also, is the reverse case safe, does an older server failing to read the additional value (seemingly always sent now) have potential to lead to any issues on older servers, i.e how might the buffer continue to be used later if at all? Should the client omit the value for older servers? (Or does the presumed version change prevent the new client working with the old server anyway? I don't know how that stuff is handled, just commenting from reading the diff here).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245953999
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java ---
    @@ -52,6 +57,7 @@ public String toString() {
           StringBuffer buff = new StringBuffer(getParentString());
           buff.append(", queueName=" + queueName);
           buff.append(", filterString=" + filterString);
    +      buff.append(", priority=" + priority);
    --- End diff --
   
    Nitpicking, the other details seem to be emitted 'in order' relative to the buffer content, so would it make sense to put this at the end consistent with its location?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245965929
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java ---
    @@ -233,6 +239,11 @@ public Object createSender(ProtonServerSenderContext protonSender,
           return consumer;
        }
     
    +   private int getPriority(Map<Symbol, Object> properties) {
    +      Integer value = properties == null ? null : (Integer) properties.get(PRIORITY);
    --- End diff --
   
    Comments on the original #2488 PR suggest you want to align with Qpid Broker-J in this area. Its support (and the accompanying documentation lift) notes as an integral value, so the value here is not necessarily going to be the Integer type.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245968414
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/ConsumerPriorityTest.java ---
    @@ -0,0 +1,298 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements. See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License. You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.jms.client;
    +
    +import org.apache.activemq.artemis.api.core.RoutingType;
    +import org.apache.activemq.artemis.api.core.SimpleString;
    +import org.apache.activemq.artemis.jms.client.ActiveMQDestination;
    +import org.apache.activemq.artemis.tests.util.JMSTestBase;
    +import org.junit.Before;
    +import org.junit.Test;
    +
    +import javax.jms.Connection;
    +import javax.jms.ConnectionFactory;
    +import javax.jms.Destination;
    +import javax.jms.MessageConsumer;
    +import javax.jms.MessageProducer;
    +import javax.jms.Queue;
    +import javax.jms.Session;
    +import javax.jms.TextMessage;
    +import javax.jms.Topic;
    +
    +/**
    + * Exclusive Test
    + */
    +public class ConsumerPriorityTest extends JMSTestBase {
    +
    +   private SimpleString queueName = SimpleString.toSimpleString("jms.consumer.priority.queue");
    --- End diff --
   
    Rather than hard coding a shared name, using the test name for the queue name is nice as it isolates different tests and makes the relationship clear, sometimes makes it easier to work on issues later with particular tests. There is a test name rule in the parent class, and a getName() method that can be used with it.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245972322
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/amq/QueueConsumerPriorityTest.java ---
    @@ -0,0 +1,65 @@
    +/**
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.openwire.amq;
    +
    +import javax.jms.JMSException;
    +import javax.jms.Message;
    +import javax.jms.MessageConsumer;
    +import javax.jms.MessageProducer;
    +import javax.jms.Session;
    +
    +
    +import org.apache.activemq.artemis.tests.integration.openwire.BasicOpenWireTest;
    +import org.apache.activemq.command.ActiveMQQueue;
    +import org.junit.Before;
    +import org.junit.Test;
    +
    +public class QueueConsumerPriorityTest extends BasicOpenWireTest {
    +
    +
    +   @Override
    +   @Before
    +   public void setUp() throws Exception {
    +      super.setUp();
    +      this.makeSureCoreQueueExist("QUEUE.A");
    +   }
    +   @Test
    +   public void testQueueConsumerPriority() throws JMSException, InterruptedException {
    +      connection.start();
    +      Session consumerLowPriority = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      Session consumerHighPriority = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      assertNotNull(consumerHighPriority);
    +      Session senderSession = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      String queueName = "QUEUE.A";
    +      ActiveMQQueue low = new ActiveMQQueue(queueName + "?consumer.priority=1");
    +      MessageConsumer lowConsumer = consumerLowPriority.createConsumer(low);
    +
    +      ActiveMQQueue high = new ActiveMQQueue(queueName + "?consumer.priority=2");
    +      MessageConsumer highConsumer = consumerLowPriority.createConsumer(high);
    +
    +      ActiveMQQueue senderQueue = new ActiveMQQueue(queueName);
    +
    +      MessageProducer producer = senderSession.createProducer(senderQueue);
    +
    +      Message msg = senderSession.createTextMessage("test");
    +      for (int i = 0; i < 1000; i++) {
    +         producer.send(msg);
    +         assertNotNull("null on iteration: " + i, highConsumer.receive(1000));
    +      }
    +      assertNull(lowConsumer.receive(2000));
    --- End diff --
   
    Would a receiveNoWait (either in or outside the loop) like the other tests be nicer than burning 2 seconds? Slow tests is a key reason eventually noone wants to runs the tests :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245974624
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *      http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.amqp;
    +
    +import org.apache.activemq.transport.amqp.client.AmqpClient;
    +import org.apache.activemq.transport.amqp.client.AmqpConnection;
    +import org.apache.activemq.transport.amqp.client.AmqpMessage;
    +import org.apache.activemq.transport.amqp.client.AmqpReceiver;
    +import org.apache.activemq.transport.amqp.client.AmqpSession;
    +import org.apache.qpid.proton.amqp.Symbol;
    +import org.junit.Test;
    +
    +import java.util.HashMap;
    +import java.util.Map;
    +import java.util.concurrent.TimeUnit;
    +
    +/**
    + * Test various behaviors of AMQP receivers with the broker.
    + */
    +public class AmqpReceiverPriorityTest extends AmqpClientTestSupport {
    +
    +   @Test(timeout = 30000)
    +   public void testPriority() throws Exception {
    +
    +      AmqpClient client = createAmqpClient();
    +      AmqpConnection connection = addConnection(client.connect());
    +      AmqpSession session = connection.createSession();
    +
    +      Map<Symbol, Object> properties1 = new HashMap<>();
    +      properties1.put(Symbol.getSymbol("priority"), 50);
    --- End diff --
   
    I'd suggest creating consumers with priorities out of order (e.g highest in middle), so they arent simply registered in sequence, as otherwise a simple failure to round-robin delivery attempts (given every receiver has enough credit to receive all messages) might also lead to the expected result even without any priority handling consideration.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245973707
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *      http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.amqp;
    +
    +import org.apache.activemq.transport.amqp.client.AmqpClient;
    +import org.apache.activemq.transport.amqp.client.AmqpConnection;
    +import org.apache.activemq.transport.amqp.client.AmqpMessage;
    +import org.apache.activemq.transport.amqp.client.AmqpReceiver;
    +import org.apache.activemq.transport.amqp.client.AmqpSession;
    +import org.apache.qpid.proton.amqp.Symbol;
    +import org.junit.Test;
    +
    +import java.util.HashMap;
    +import java.util.Map;
    +import java.util.concurrent.TimeUnit;
    +
    +/**
    + * Test various behaviors of AMQP receivers with the broker.
    + */
    +public class AmqpReceiverPriorityTest extends AmqpClientTestSupport {
    +
    +   @Test(timeout = 30000)
    +   public void testPriority() throws Exception {
    +
    +      AmqpClient client = createAmqpClient();
    +      AmqpConnection connection = addConnection(client.connect());
    +      AmqpSession session = connection.createSession();
    +
    +      Map<Symbol, Object> properties1 = new HashMap<>();
    +      properties1.put(Symbol.getSymbol("priority"), 50);
    +      AmqpReceiver receiver1 = session.createReceiver(getQueueName(), null, false, false, properties1);
    +      receiver1.flow(100);
    +
    +      Map<Symbol, Object> properties2 = new HashMap<>();
    +      properties2.put(Symbol.getSymbol("priority"), 10);
    +      AmqpReceiver receiver2 = session.createReceiver(getQueueName(), null, false, false, properties2);
    +      receiver2.flow(100);
    +
    +      Map<Symbol, Object> properties3 = new HashMap<>();
    +      properties3.put(Symbol.getSymbol("priority"), 5);
    +      AmqpReceiver receiver3 = session.createReceiver(getQueueName(), null, false, false, properties3);
    +      receiver3.flow(100);
    +
    +      sendMessages(getQueueName(), 5);
    +
    +
    +      for (int i = 0; i < 5; i++) {
    +         AmqpMessage message1 = receiver1.receive(250, TimeUnit.MILLISECONDS);
    +         AmqpMessage message2 = receiver2.receive(250, TimeUnit.MILLISECONDS);
    +         AmqpMessage message3 = receiver3.receive(250, TimeUnit.MILLISECONDS);
    +         assertNotNull("did not receive message first time", message1);
    +         assertEquals("MessageID:" + i, message1.getMessageId());
    +         message1.accept();
    +         assertNull("message is not meant to goto lower priority receiver", message2);
    +         assertNull("message is not meant to goto lower priority receiver", message3);
    +      }
    +
    +      //Close the high priority receiver
    +      receiver1.close();
    +
    +      sendMessages(getQueueName(), 5);
    +
    +      //Check messages now goto next priority receiver
    +      for (int i = 0; i < 5; i++) {
    +         AmqpMessage message2 = receiver2.receive(250, TimeUnit.MILLISECONDS);
    +         AmqpMessage message3 = receiver3.receive(250, TimeUnit.MILLISECONDS);
    --- End diff --
   
    As above.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r245973668
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *      http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.amqp;
    +
    +import org.apache.activemq.transport.amqp.client.AmqpClient;
    +import org.apache.activemq.transport.amqp.client.AmqpConnection;
    +import org.apache.activemq.transport.amqp.client.AmqpMessage;
    +import org.apache.activemq.transport.amqp.client.AmqpReceiver;
    +import org.apache.activemq.transport.amqp.client.AmqpSession;
    +import org.apache.qpid.proton.amqp.Symbol;
    +import org.junit.Test;
    +
    +import java.util.HashMap;
    +import java.util.Map;
    +import java.util.concurrent.TimeUnit;
    +
    +/**
    + * Test various behaviors of AMQP receivers with the broker.
    + */
    +public class AmqpReceiverPriorityTest extends AmqpClientTestSupport {
    +
    +   @Test(timeout = 30000)
    +   public void testPriority() throws Exception {
    +
    +      AmqpClient client = createAmqpClient();
    +      AmqpConnection connection = addConnection(client.connect());
    +      AmqpSession session = connection.createSession();
    +
    +      Map<Symbol, Object> properties1 = new HashMap<>();
    +      properties1.put(Symbol.getSymbol("priority"), 50);
    +      AmqpReceiver receiver1 = session.createReceiver(getQueueName(), null, false, false, properties1);
    +      receiver1.flow(100);
    +
    +      Map<Symbol, Object> properties2 = new HashMap<>();
    +      properties2.put(Symbol.getSymbol("priority"), 10);
    +      AmqpReceiver receiver2 = session.createReceiver(getQueueName(), null, false, false, properties2);
    +      receiver2.flow(100);
    +
    +      Map<Symbol, Object> properties3 = new HashMap<>();
    +      properties3.put(Symbol.getSymbol("priority"), 5);
    +      AmqpReceiver receiver3 = session.createReceiver(getQueueName(), null, false, false, properties3);
    +      receiver3.flow(100);
    +
    +      sendMessages(getQueueName(), 5);
    +
    +
    +      for (int i = 0; i < 5; i++) {
    +         AmqpMessage message1 = receiver1.receive(250, TimeUnit.MILLISECONDS);
    +         AmqpMessage message2 = receiver2.receive(250, TimeUnit.MILLISECONDS);
    --- End diff --
   
    Burning 250ms twice per loop seems excessive. There is a receiveNoWait that could be used for initial verification nothing arrived, and/or a small final timed wait could be done outside the loop afterwards. Alternatively, pullImmediate() would avoid unnecessary waiting.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246127864
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java ---
    @@ -52,6 +57,7 @@ public String toString() {
           StringBuffer buff = new StringBuffer(getParentString());
           buff.append(", queueName=" + queueName);
           buff.append(", filterString=" + filterString);
    +      buff.append(", priority=" + priority);
    --- End diff --
   
    Makes sense


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246128143
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java ---
    @@ -104,6 +119,11 @@ public void decodeRest(final ActiveMQBuffer buffer) {
           filterString = buffer.readNullableSimpleString();
           browseOnly = buffer.readBoolean();
           requiresResponse = buffer.readBoolean();
    +      if (buffer.readableBytes() > 0) {
    --- End diff --
   
    This is typical pattern used for adding safely a new field that can be either nullable or defaultable. Used many times over.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246128551
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpReceiverPriorityTest.java ---
    @@ -0,0 +1,90 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *      http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.amqp;
    +
    +import org.apache.activemq.transport.amqp.client.AmqpClient;
    +import org.apache.activemq.transport.amqp.client.AmqpConnection;
    +import org.apache.activemq.transport.amqp.client.AmqpMessage;
    +import org.apache.activemq.transport.amqp.client.AmqpReceiver;
    +import org.apache.activemq.transport.amqp.client.AmqpSession;
    +import org.apache.qpid.proton.amqp.Symbol;
    +import org.junit.Test;
    +
    +import java.util.HashMap;
    +import java.util.Map;
    +import java.util.concurrent.TimeUnit;
    +
    +/**
    + * Test various behaviors of AMQP receivers with the broker.
    + */
    +public class AmqpReceiverPriorityTest extends AmqpClientTestSupport {
    +
    +   @Test(timeout = 30000)
    +   public void testPriority() throws Exception {
    +
    +      AmqpClient client = createAmqpClient();
    +      AmqpConnection connection = addConnection(client.connect());
    +      AmqpSession session = connection.createSession();
    +
    +      Map<Symbol, Object> properties1 = new HashMap<>();
    +      properties1.put(Symbol.getSymbol("priority"), 50);
    --- End diff --
   
    This is actually tested on the queueconsumerimpl test. But agree we can do same here


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246132135
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/openwire/amq/QueueConsumerPriorityTest.java ---
    @@ -0,0 +1,65 @@
    +/**
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.openwire.amq;
    +
    +import javax.jms.JMSException;
    +import javax.jms.Message;
    +import javax.jms.MessageConsumer;
    +import javax.jms.MessageProducer;
    +import javax.jms.Session;
    +
    +
    +import org.apache.activemq.artemis.tests.integration.openwire.BasicOpenWireTest;
    +import org.apache.activemq.command.ActiveMQQueue;
    +import org.junit.Before;
    +import org.junit.Test;
    +
    +public class QueueConsumerPriorityTest extends BasicOpenWireTest {
    +
    +
    +   @Override
    +   @Before
    +   public void setUp() throws Exception {
    +      super.setUp();
    +      this.makeSureCoreQueueExist("QUEUE.A");
    +   }
    +   @Test
    +   public void testQueueConsumerPriority() throws JMSException, InterruptedException {
    +      connection.start();
    +      Session consumerLowPriority = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      Session consumerHighPriority = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      assertNotNull(consumerHighPriority);
    +      Session senderSession = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
    +      String queueName = "QUEUE.A";
    +      ActiveMQQueue low = new ActiveMQQueue(queueName + "?consumer.priority=1");
    +      MessageConsumer lowConsumer = consumerLowPriority.createConsumer(low);
    +
    +      ActiveMQQueue high = new ActiveMQQueue(queueName + "?consumer.priority=2");
    +      MessageConsumer highConsumer = consumerLowPriority.createConsumer(high);
    +
    +      ActiveMQQueue senderQueue = new ActiveMQQueue(queueName);
    +
    +      MessageProducer producer = senderSession.createProducer(senderQueue);
    +
    +      Message msg = senderSession.createTextMessage("test");
    +      for (int i = 0; i < 1000; i++) {
    +         producer.send(msg);
    +         assertNotNull("null on iteration: " + i, highConsumer.receive(1000));
    +      }
    +      assertNull(lowConsumer.receive(2000));
    --- End diff --
   
    this is the original test from ActiveMQ5 i was trying to keep this test as much un-touched as possible to ensure behavior is the same.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246133832
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/ConsumerPriorityTest.java ---
    @@ -0,0 +1,298 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements. See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License. You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.activemq.artemis.tests.integration.jms.client;
    +
    +import org.apache.activemq.artemis.api.core.RoutingType;
    +import org.apache.activemq.artemis.api.core.SimpleString;
    +import org.apache.activemq.artemis.jms.client.ActiveMQDestination;
    +import org.apache.activemq.artemis.tests.util.JMSTestBase;
    +import org.junit.Before;
    +import org.junit.Test;
    +
    +import javax.jms.Connection;
    +import javax.jms.ConnectionFactory;
    +import javax.jms.Destination;
    +import javax.jms.MessageConsumer;
    +import javax.jms.MessageProducer;
    +import javax.jms.Queue;
    +import javax.jms.Session;
    +import javax.jms.TextMessage;
    +import javax.jms.Topic;
    +
    +/**
    + * Exclusive Test
    + */
    +public class ConsumerPriorityTest extends JMSTestBase {
    +
    +   private SimpleString queueName = SimpleString.toSimpleString("jms.consumer.priority.queue");
    --- End diff --
   
    Nice i didnt know about that in the parent class. will change to use this..


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2490: V2 196

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2490#discussion_r246135542
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionCreateConsumerMessage.java ---
    @@ -52,6 +57,7 @@ public String toString() {
           StringBuffer buff = new StringBuffer(getParentString());
           buff.append(", queueName=" + queueName);
           buff.append(", filterString=" + filterString);
    +      buff.append(", priority=" + priority);
    --- End diff --
   
    done


---
1234