[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242788099
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
---
@@ -469,20 +530,17 @@ public void close(ErrorCondition condition) throws
ActiveMQAMQPException {
sender.setCondition(condition);
}
protonSession.removeSender(sender);
- connection.lock();
- try {
- sender.close();
- } finally {
- connection.unlock();
- }
- connection.flush();
- try {
- sessionSPI.closeSender(brokerConsumer);
- } catch (Exception e) {
- log.warn(e.getMessage(), e);
- throw new ActiveMQAMQPInternalErrorException(e.getMessage());
- }
+ connection.runLater(() -> {
+ sender.close();
+ try {
+sessionSPI.closeSender(brokerConsumer);
+ } catch (Exception e) {
+log.warn(e.getMessage(), e);
--- End diff --
Static logger method needed
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242788013
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
---
@@ -730,22 +793,29 @@ public int deliverMessage(MessageReference
messageReference, int deliveryCount,
if (preSettle) {
// Presettled means the client implicitly accepts any
delivery we send it.
- sessionSPI.ack(null, brokerConsumer,
messageReference.getMessage());
+ try {
+ sessionSPI.ack(null, brokerConsumer,
messageReference.getMessage());
+ } catch (Exception e) {
+ log.debug(e.getMessage(), e);
+ }
delivery.settle();
} else {
sender.advance();
}
connection.flush();
} finally {
-connection.unlock();
+synchronized (creditsLock) {
+ pending.decrementAndGet();
+}
+if (releaseRequired) {
+ ((NettyReadable) sendBuffer).getByteBuf().release();
+}
}
+ } catch (Exception e) {
+ log.warn(e.getMessage(), e);
--- End diff --
Static logger method needed
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242787638
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
---
@@ -359,59 +371,77 @@ public boolean flowControl(ReadyListener
readyListener) {
@Override
public void onRemoteOpen(Connection connection) throws Exception {
- lock();
+ handler.requireHandler();
try {
- try {
-initInternal();
- } catch (Exception e) {
-log.error("Error init connection", e);
- }
- if (!validateConnection(connection)) {
-connection.close();
- } else {
-connection.setContext(AMQPConnectionContext.this);
-connection.setContainer(containerId);
-connection.setProperties(connectionProperties);
-
connection.setOfferedCapabilities(getConnectionCapabilitiesOffered());
-connection.open();
- }
- } finally {
- unlock();
+ initInternal();
+ } catch (Exception e) {
+ log.error("Error init connection", e);
+ }
+ if (!validateConnection(connection)) {
+ connection.close();
+ } else {
+ connection.setContext(AMQPConnectionContext.this);
+ connection.setContainer(containerId);
+ connection.setProperties(connectionProperties);
+
connection.setOfferedCapabilities(getConnectionCapabilitiesOffered());
+ connection.open();
}
initialise();
- /*
- * This can be null which is in effect an empty map, also we
really don't need to check this for in bound connections
- * but its here in case we add support for outbound connections.
- * */
+ /*
+ * This can be null which is in effect an empty map, also we really
don't need to check this for in bound connections
+ * but its here in case we add support for outbound connections.
+ * */
if (connection.getRemoteProperties() == null ||
!connection.getRemoteProperties().containsKey(CONNECTION_OPEN_FAILED)) {
long nextKeepAliveTime = handler.tick(true);
if (nextKeepAliveTime != 0 && scheduledPool != null) {
-scheduledPool.schedule(new Runnable() {
- @Override
- public void run() {
- Long rescheduleAt = handler.tick(false);
- if (rescheduleAt == null) {
- // this mean tick could not acquire a lock, we will
just retry in 10 milliseconds.
- scheduledPool.schedule(this, 10,
TimeUnit.MILLISECONDS);
- } else if (rescheduleAt != 0) {
- scheduledPool.schedule(this, rescheduleAt -
TimeUnit.NANOSECONDS.toMillis(System.nanoTime()), TimeUnit.MILLISECONDS);
- }
- }
-}, (nextKeepAliveTime -
TimeUnit.NANOSECONDS.toMillis(System.nanoTime())), TimeUnit.MILLISECONDS);
+scheduledPool.schedule(new ScheduleRunnable(),
(nextKeepAliveTime - TimeUnit.NANOSECONDS.toMillis(System.nanoTime())),
TimeUnit.MILLISECONDS);
}
}
}
+ class TickerRunnable implements Runnable {
+
+ final ScheduleRunnable scheduleRunnable;
+
+ TickerRunnable(ScheduleRunnable scheduleRunnable) {
+ this.scheduleRunnable = scheduleRunnable;
+ }
+
+ @Override
+ public void run() {
+ try {
+Long rescheduleAt = handler.tick(false);
+if (rescheduleAt == null) {
+ // this mean tick could not acquire a lock, we will just
retry in 10 milliseconds.
+ scheduledPool.schedule(scheduleRunnable, 10,
TimeUnit.MILLISECONDS);
+} else if (rescheduleAt != 0) {
+ scheduledPool.schedule(scheduleRunnable, rescheduleAt -
TimeUnit.NANOSECONDS.toMillis(System.nanoTime()), TimeUnit.MILLISECONDS);
+}
+ } catch (Exception e) {
+log.warn(e.getMessage(), e);
+ }
--- End diff --
This should have a static logger method with a code
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242787783
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
---
@@ -122,7 +136,53 @@ public Object getBrokerConsumer() {
@Override
public void onFlow(int currentCredits, boolean drain) {
- sessionSPI.onFlowConsumer(brokerConsumer, currentCredits, drain);
+ connection.requireInHandler();
+
+ setupCredit();
+
+ ServerConsumerImpl serverConsumer = (ServerConsumerImpl)
brokerConsumer;
+ if (drain) {
+ // If the draining is already running, then don't do anything
+ if (draining.compareAndSet(false, true)) {
+final ProtonServerSenderContext plugSender =
(ProtonServerSenderContext) serverConsumer.getProtocolContext();
+serverConsumer.forceDelivery(1, new Runnable() {
+ @Override
+ public void run() {
+ try {
+ connection.runNow(() -> {
+plugSender.reportDrained();
+setupCredit();
+ });
+ } finally {
+ draining.set(false);
+ }
+ }
+});
+ }
+ } else {
+ serverConsumer.receiveCredits(-1);
+ }
+ }
+
+ public boolean hasCredits() {
+ if (!connection.flowControl(onflowControlReady)) {
+ return false;
+ }
+
+ //return true;
+ //return getSender().getCredit() > 0;
--- End diff --
Remove commented out code
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242788241
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
---
@@ -730,22 +793,29 @@ public int deliverMessage(MessageReference
messageReference, int deliveryCount,
if (preSettle) {
// Presettled means the client implicitly accepts any
delivery we send it.
- sessionSPI.ack(null, brokerConsumer,
messageReference.getMessage());
+ try {
+ sessionSPI.ack(null, brokerConsumer,
messageReference.getMessage());
+ } catch (Exception e) {
+ log.debug(e.getMessage(), e);
+ }
delivery.settle();
} else {
sender.advance();
}
connection.flush();
} finally {
-connection.unlock();
+synchronized (creditsLock) {
+ pending.decrementAndGet();
+}
+if (releaseRequired) {
+ ((NettyReadable) sendBuffer).getByteBuf().release();
+}
}
+ } catch (Exception e) {
+ log.warn(e.getMessage(), e);
- return size;
- } finally {
- if (releaseRequired) {
-((NettyReadable) sendBuffer).getByteBuf().release();
- }
+ // important todo: Error treatment
--- End diff --
Looks like some work left todo here
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242787574
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
---
@@ -471,40 +494,42 @@ public void onFlow(Link link) throws Exception {
@Override
public void onRemoteClose(Link link) throws Exception {
- lock();
- try {
+ handler.requireHandler();
+
+ // We scheduled it for later, as that will work through anything
that's pending on the current deliveries.
+ runLater(() -> {
link.close();
link.free();
- } finally {
- unlock();
- }
- ProtonDeliveryHandler linkContext = (ProtonDeliveryHandler)
link.getContext();
- if (linkContext != null) {
- linkContext.close(true);
- }
+ ProtonDeliveryHandler linkContext = (ProtonDeliveryHandler)
link.getContext();
+ if (linkContext != null) {
+try {
+ linkContext.close(true);
+} catch (Exception e) {
+ log.error(e.getMessage(), e);
--- End diff --
this should have a static logger method
---
[GitHub] activemq-artemis pull request #2471: ARTEMIS-2207 Page Showing Log.warns for...
Github user asfgit closed the pull request at: https://github.com/apache/activemq-artemis/pull/2471 ---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user clebertsuconic commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2467#discussion_r242745363 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java --- @@ -159,7 +160,7 @@ private final SimpleString managementAddress; - protected final RoutingContext routingContext = new RoutingContextImpl(null); + protected final RoutingContext _routingContext; --- End diff -- @franz1981 I reverted the change that's not needed any longer. all amended. ---
[GitHub] activemq-artemis pull request #2471: ARTEMIS-2207 Page Showing Log.warns for...
GitHub user clebertsuconic opened a pull request: https://github.com/apache/activemq-artemis/pull/2471 ARTEMIS-2207 Page Showing Log.warns for regular acked messages You can merge this pull request into a Git repository by running: $ git pull https://github.com/clebertsuconic/activemq-artemis paging Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2471.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 #2471 commit 9984b638af54ac6c2270604e046d739ed6b6f328 Author: Clebert Suconic Date: 2018-12-18T23:17:03Z ARTEMIS-2207 Page Showing Log.warns for regular acked messages ---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user clebertsuconic commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2467#discussion_r242727332 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java --- @@ -159,7 +160,7 @@ private final SimpleString managementAddress; - protected final RoutingContext routingContext = new RoutingContextImpl(null); + protected final RoutingContext _routingContext; --- End diff -- @franz1981 I don't follow you ---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242727059
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ExecutorNettyAdapter.java
---
@@ -0,0 +1,221 @@
+/*
+ * Copyright 2005-2014 Red Hat, Inc.
+ * Red Hat 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.protocol.amqp.proton.handler;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelPromise;
+import io.netty.channel.EventLoop;
+import io.netty.channel.EventLoopGroup;
+import io.netty.util.concurrent.EventExecutor;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.ProgressivePromise;
+import io.netty.util.concurrent.Promise;
+import io.netty.util.concurrent.ScheduledFuture;
+import org.apache.activemq.artemis.utils.actors.ArtemisExecutor;
+
+/** Test cases may supply a simple executor instead of the real Netty
Executor
+ * On that case this is a simple adapter for what's needed from these
tests.
+ * Not intended to be used in production.
+ *
+ * TODO: This could be refactored out of the main codebase but at a high
cost.
+ *We may do it some day if we find an easy way that won't clutter
the code too much.
+ * */
+public class ExecutorNettyAdapter implements EventLoop {
--- End diff --
@franz1981 this is basically for unit testing, I did not want to invest a
lot of code on it.
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242725348
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/ActiveMQProtonRemotingConnection.java
---
@@ -119,10 +119,15 @@ public boolean isDestroyed() {
@Override
public void disconnect(boolean criticalError) {
- ErrorCondition errorCondition = new ErrorCondition();
- errorCondition.setCondition(AmqpSupport.CONNECTION_FORCED);
- amqpConnection.close(errorCondition);
- getTransportConnection().close();
+ amqpConnection.runLater(() -> {
+ ErrorCondition errorCondition = new ErrorCondition();
+ errorCondition.setCondition(AmqpSupport.CONNECTION_FORCED);
+ amqpConnection.close(errorCondition);
+ amqpConnection.flush();
+ });
+ amqpConnection.runLater(() -> {
--- End diff --
@franz1981 During my work, the first one was runNow, the second Later. (I
would accept better names BTW)
later I changed the first to later and forgot the other one :)
fixing it on my next ammend
---
[GitHub] activemq-artemis issue #2199: ARTEMIS-1996 MappedSequentialFileFactory may c...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2199 IMO this PR should be closed based on the commits from #2250. @morefuntang, can you close it? ---
[GitHub] activemq-artemis issue #2467: ARTEMIS-2205 Performance improvements on AMQP ...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2467 I've read through this and it looks fine, although I can't say I understood it all. :) ---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user franz1981 commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2467#discussion_r242700956 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java --- @@ -159,7 +160,7 @@ private final SimpleString managementAddress; - protected final RoutingContext routingContext = new RoutingContextImpl(null); + protected final RoutingContext _routingContext; --- End diff -- we can use just `routingContext` name, dropping `_` ---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242702260
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ExecutorNettyAdapter.java
---
@@ -0,0 +1,221 @@
+/*
+ * Copyright 2005-2014 Red Hat, Inc.
+ * Red Hat 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.protocol.amqp.proton.handler;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelPromise;
+import io.netty.channel.EventLoop;
+import io.netty.channel.EventLoopGroup;
+import io.netty.util.concurrent.EventExecutor;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.ProgressivePromise;
+import io.netty.util.concurrent.Promise;
+import io.netty.util.concurrent.ScheduledFuture;
+import org.apache.activemq.artemis.utils.actors.ArtemisExecutor;
+
+/** Test cases may supply a simple executor instead of the real Netty
Executor
+ * On that case this is a simple adapter for what's needed from these
tests.
+ * Not intended to be used in production.
+ *
+ * TODO: This could be refactored out of the main codebase but at a high
cost.
+ *We may do it some day if we find an easy way that won't clutter
the code too much.
+ * */
+public class ExecutorNettyAdapter implements EventLoop {
--- End diff --
Instead of using `ExecutorNettyAdapter` we couldn't reuse `OrderedExecutor`
interface adding `inEventLoop()` on it?
`OrderedExecutor::inEventLoop()` could just be `== inHandler()`
---
[GitHub] activemq-artemis pull request #2467: ARTEMIS-2205 Performance improvements o...
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2467#discussion_r242686500
--- Diff:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/ActiveMQProtonRemotingConnection.java
---
@@ -119,10 +119,15 @@ public boolean isDestroyed() {
@Override
public void disconnect(boolean criticalError) {
- ErrorCondition errorCondition = new ErrorCondition();
- errorCondition.setCondition(AmqpSupport.CONNECTION_FORCED);
- amqpConnection.close(errorCondition);
- getTransportConnection().close();
+ amqpConnection.runLater(() -> {
+ ErrorCondition errorCondition = new ErrorCondition();
+ errorCondition.setCondition(AmqpSupport.CONNECTION_FORCED);
+ amqpConnection.close(errorCondition);
+ amqpConnection.flush();
+ });
+ amqpConnection.runLater(() -> {
--- End diff --
Probably it is a naive question, but why 2 separate runLater calls instead
of one?
---
[GitHub] activemq-artemis issue #1793: ARTEMIS-1498: Openwire internal headers should...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/1793 @RaiSaurabh, at this point there are lots of conflicts on this PR. They either need to be fixed and this PR can move forward or it needs to be closed. ---
[GitHub] activemq-artemis pull request #2290: Add LGTM code quality badges
Github user xcorail closed the pull request at: https://github.com/apache/activemq-artemis/pull/2290 ---
[GitHub] activemq-artemis issue #2290: Add LGTM code quality badges
Github user xcorail commented on the issue: https://github.com/apache/activemq-artemis/pull/2290 Hello @jbertram Done ---
[GitHub] activemq-artemis issue #2290: Add LGTM code quality badges
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2290 I don't think there's currently any plan to merge this. Could you close this PR, @xcorail? ---
[GitHub] activemq-artemis pull request #2470: Fixes for alerts from lgtm.com
GitHub user jbertram opened a pull request: https://github.com/apache/activemq-artemis/pull/2470 Fixes for alerts from lgtm.com After reviewing lgtm.com based on #2290 I decided to fix a handful of the warnings and errors they reported. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jbertram/activemq-artemis lgtm Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2470.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 #2470 commit 99e69b27a0a117d3c5736968ff1c542d2a57004d Author: Justin Bertram Date: 2018-09-05T18:49:13Z NO-JIRA fix lgtm.com warnings Warnings enumerated at https://lgtm.com/projects/g/apache/activemq-artemis/alerts/?mode=tree=warning commit e71ff0826cd17544f4b264354ccb54cc033e381e Author: Justin Bertram Date: 2018-09-06T03:04:03Z NO-JIRA fix lgtm.com errors Errors enumerated at https://lgtm.com/projects/g/apache/activemq-artemis/alerts/?mode=tree=error ---
[GitHub] activemq-artemis pull request #2458: ARTEMIS-2199 PagingStore leak when dele...
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2458#discussion_r242640005
--- Diff:
artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/impl/JMSServerManagerImpl.java
---
@@ -1719,11 +1720,11 @@ public void callback(SimpleString queueName) throws
Exception {
@Override
public void callback(SimpleString address, SimpleString queueName)
throws Exception {
Queue queue = server.locateQueue(address);
- Collection bindings =
server.getPostOffice().getBindingsForAddress(address).getBindings();
--- End diff --
I opened #2469 as a result of my investigation on the master branch.
---
[GitHub] activemq-artemis pull request #2469: NO-JIRA avoid unnecessary Bindings inst...
GitHub user jbertram opened a pull request: https://github.com/apache/activemq-artemis/pull/2469 NO-JIRA avoid unnecessary Bindings instance creation When trying to get the bindings for an address the getBindingsForAddress method will create a Bindings instance if there are no bindings for the address. This is unnecessary in most circumstances so use the lookupBindingsForAddress method instead and check for null. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jbertram/activemq-artemis avoidBindingsInstance Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2469.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 #2469 commit 9fe86f3507a4b483a131c0af9ead62d8f77a0efa Author: Justin Bertram Date: 2018-12-18T17:48:12Z NO-JIRA avoid unnecessary Bindings instance creation When trying to get the bindings for an address the getBindingsForAddress method will create a Bindings instance if there are no bindings for the address. This is unnecessary in most circumstances so use the lookupBindingsForAddress method instead and check for null. ---
[GitHub] activemq-artemis issue #2466: ARTEMIS-2206 The MQTT consumer reconnection ca...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 If you don't know git, do some googling about squash and interactive rebase. git push -f will update the Pull Request. ---
[GitHub] activemq-artemis issue #2466: ARTEMIS-2206 The MQTT consumer reconnection ca...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 you do this on your branch: git rebase -i HEAD~4 for each line, squash all 3 later commits into the first one, Once you squash you will be sent to a vim (or another editor you configured) where you can change the tittle. make the title reflect the JIRA, git push origin -f (your branch name) That will be enough to update this PR. ---
[GitHub] activemq-artemis pull request #1761: [ARTEMIS-1527] - [Artemis Testsuite] Ac...
Github user JiriOndrusek closed the pull request at: https://github.com/apache/activemq-artemis/pull/1761 ---
[GitHub] activemq-artemis issue #1761: [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQM...
Github user JiriOndrusek commented on the issue: https://github.com/apache/activemq-artemis/pull/1761 I agree, closing as requested in previous comment. ---
[GitHub] activemq-artemis issue #1761: [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQM...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/1761 @JiriOndrusek, I don't think this is going to get merged. Can you go ahead and close this PR? ---
Re: HEADS-UP ActiveMQ artemis 2.6.4 release
I was going to release today, however these tests are failing on the 2.6.x branch: org.apache.activemq.artemis.tests.integration.management.QueueControlTest.testGetMessagesAdded[durable=false] org.apache.activemq.artemis.tests.integration.openwire.OpenWireGroupingTest.testGrouping[core-send=true core-receive=true] org.apache.activemq.artemis.tests.integration.openwire.OpenWireGroupingTest.testGrouping[core-send=false core-receive=true] org.apache.activemq.artemis.tests.integration.openwire.cluster.TemporaryQueueClusterTest.testClusteredQueue On Tue, Dec 18, 2018 at 6:21 AM Christopher Shannon wrote: > > I agree that 2.7.0 can wait until January at this point because of the > holidays coming up for a lot of people however it is way past due in my > opinion. > > In general I would like to see Artemis released much more frequently than > it currently is. Artemis is very active with constant new features and bug > fixes so we should be releasing consistently to get things out there so > people can use them. With as much stuff going into it every day we could > probably do something like a monthly release and have plenty to show for it. > > I don't really see a reason to hold up a release for any new feature (bug > fixes are different of course) because there are already so many features > that are done that haven't been released. For example, I added support for > changing the defaultConsumerWindowSize per address back in October and at > least 3 people have asked about that feature and I keep having to tell them > it's not out yet. > > If AMQP large message support isn't quite ready then no big deal...you just > put it into 2.8.0 and release it relatively soon after. There's no reason > a feature has to be tied to a specific version when you can always just > increase the version number by 1 (the exception being breaking changes > obviously and major things that would make more sense in a major version > number change) > > On Mon, Dec 17, 2018 at 1:43 PM Clebert Suconic > wrote: > > > Thanks. i had a heads up before on another thread but I couldn’t do > > because of my workload on the task I was working on. > > > > > > I will cut today and the vote thread sent tomorrow morning or later > > tonight. > > > > I personally want 2.7.0 out early next year. I will work proactively on > > that (and welcome anyone’s help on that) > > > > It would be nice to have large message streamed in AMQP as we convert to > > core right now. That’s the main thing I held a 2.7.0 release before. If I > > can’t do it I will still do it. But if anyone is willing to help here it > > would be awesome. > > > > > > On Mon, Dec 17, 2018 at 11:57 AM Robbie Gemmell > > wrote: > > > > > I'd say it has already been too long since the last one so I wouldnt > > > wait until next year. A heads up is nice, and somewhat required when > > > its been a long cycle and especially so if previous heads up have been > > > missed. In this case I'd personally say it would be fine to proceed > > > even later today if it seems ready and noone specifically objects, so > > > it is out before people start to disappear for holidays. Other > > > versions numbers are still available if anyone needs to do another > > > release after. > > > > > > Robbie > > > > > > On Mon, 17 Dec 2018 at 16:03, Clebert Suconic > > > > > wrote: > > > > > > > > Would be too bad if I cut this tomorrow? So we have time for a vote > > > before > > > > the Christmas week. Or anyone would rather have it next year ? > > > > > > > > I had promised few weeks back but i was busy with a big chunk of work > > > > around performance on AMQP. > > > > -- > > > > Clebert Suconic > > > > > -- > > Clebert Suconic > > -- Clebert Suconic
[GitHub] activemq-artemis issue #2466: ARTEMIS-2206 The MQTT consumer reconnection ca...
Github user onlyMIT commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 @clebertsuconic this is my first pull request on GitHubãThank you for your understanding, I will try to use JIRA to submit the code, if you find the problem, I hope you correct me. ---
[GitHub] activemq-artemis issue #2468: Add CR in sanity check
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2468 Could you create a JIRA for this at https://issues.apache.org/jira/projects/ARTEMIS and also add a test to reproduce the failure you were seeing without the fix? Something simple in `org.apache.activemq.artemis.tests.integration.stomp.StompTest` should suffice. ---
[GitHub] activemq-artemis pull request #2466: ARTEMIS-2206 The MQTT consumer reconnec...
Github user onlyMIT commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2466#discussion_r242583621 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java --- @@ -113,32 +113,7 @@ import org.apache.activemq.artemis.core.security.SecurityAuth; import org.apache.activemq.artemis.core.security.SecurityStore; import org.apache.activemq.artemis.core.security.impl.SecurityStoreImpl; -import org.apache.activemq.artemis.core.server.ActivateCallback; --- End diff -- Thanks, this is my IDE problem, I will pay attention to it. ---
[GitHub] activemq-artemis pull request #2468: Add CR in sanity check
GitHub user BiNZGi opened a pull request: https://github.com/apache/activemq-artemis/pull/2468 Add CR in sanity check Specified in https://stomp.github.io/stomp-specification-1.2.html#STOMP_Frames also carriage return (octet 13) is allowed: > The frame starts with a command string terminated by an end-of-line (EOL), which consists of an OPTIONAL carriage return (octet 13) followed by a REQUIRED line feed (octet 10). You can merge this pull request into a Git repository by running: $ git pull https://github.com/BiNZGi/activemq-artemis patch-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/activemq-artemis/pull/2468.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 #2468 commit 30a45a5b8f591fcad3eb9c105fffbdf6b4986cfa Author: BiNZGi Date: 2018-12-18T15:29:25Z Add CR in sanity check Specified in https://stomp.github.io/stomp-specification-1.2.html#STOMP_Frames also carriage return (octet 13) is allowed. ---
[GitHub] activemq-artemis pull request #2466: ARTEMIS-2206 The MQTT consumer reconnec...
Github user onlyMIT commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2466#discussion_r242582946
--- Diff:
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSubscriptionManager.java
---
@@ -194,12 +191,13 @@ private synchronized void removeSubscription(String
address) throws Exception {
SimpleString internalQueueName =
getQueueNameForTopic(internalAddress);
session.getSessionState().removeSubscription(address);
-
- ServerConsumer consumer = consumers.get(address);
+ Set queueConsumers =
session.getServer().queueConsumersQuery(internalQueueName);
--- End diff --
In the âqueueConsumersQueryâ method, use 'queueName' to query 'Binding'
and get a queue through âBindingâ. I think I am getting all the consumers
in the specified queue, not all consumers at the same address.You can check the
details of the 'queueConsumersQuery' method.
In the âActiveMQServerImpl.destroyQueueâ method, the number of
consumers on a queue is also obtained in this way.Originally I wanted to put
the shutdown consumer code here, and later found that there were multiple calls
to the âActiveMQServerImpl.destroyQueueâ method, and I abandoned the idea
of putting the consumer code off here.
---
[GitHub] activemq-artemis issue #2466: ARTEMIS-2206 The MQTT consumer reconnection ca...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 Please... squash your commits, and use the JIRA on the commit. It's kind of hard for me to review your changes here. what I saw on the first commit makes this a nogo.. but I didn't know if you fixed it on a later commit or not? ---
[GitHub] activemq-artemis pull request #2466: ARTEMIS-2206 The MQTT consumer reconnec...
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2466#discussion_r242541448
--- Diff:
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSubscriptionManager.java
---
@@ -194,12 +191,13 @@ private synchronized void removeSubscription(String
address) throws Exception {
SimpleString internalQueueName =
getQueueNameForTopic(internalAddress);
session.getSessionState().removeSubscription(address);
-
- ServerConsumer consumer = consumers.get(address);
+ Set queueConsumers =
session.getServer().queueConsumersQuery(internalQueueName);
--- End diff --
I think this is wrong. You should only check for consumers on this Session.
Your logic is closing every consumer to the same address. You keep multiple
connections and you have a failure.
---
[GitHub] activemq-artemis pull request #2466: ARTEMIS-2206 The MQTT consumer reconnec...
Github user clebertsuconic commented on a diff in the pull request: https://github.com/apache/activemq-artemis/pull/2466#discussion_r242540859 --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java --- @@ -113,32 +113,7 @@ import org.apache.activemq.artemis.core.security.SecurityAuth; import org.apache.activemq.artemis.core.security.SecurityStore; import org.apache.activemq.artemis.core.security.impl.SecurityStoreImpl; -import org.apache.activemq.artemis.core.server.ActivateCallback; --- End diff -- This will fail checkstyle. no * imports ---
Re: HEADS-UP ActiveMQ artemis 2.6.4 release
I agree that 2.7.0 can wait until January at this point because of the holidays coming up for a lot of people however it is way past due in my opinion. In general I would like to see Artemis released much more frequently than it currently is. Artemis is very active with constant new features and bug fixes so we should be releasing consistently to get things out there so people can use them. With as much stuff going into it every day we could probably do something like a monthly release and have plenty to show for it. I don't really see a reason to hold up a release for any new feature (bug fixes are different of course) because there are already so many features that are done that haven't been released. For example, I added support for changing the defaultConsumerWindowSize per address back in October and at least 3 people have asked about that feature and I keep having to tell them it's not out yet. If AMQP large message support isn't quite ready then no big deal...you just put it into 2.8.0 and release it relatively soon after. There's no reason a feature has to be tied to a specific version when you can always just increase the version number by 1 (the exception being breaking changes obviously and major things that would make more sense in a major version number change) On Mon, Dec 17, 2018 at 1:43 PM Clebert Suconic wrote: > Thanks. i had a heads up before on another thread but I couldn’t do > because of my workload on the task I was working on. > > > I will cut today and the vote thread sent tomorrow morning or later > tonight. > > I personally want 2.7.0 out early next year. I will work proactively on > that (and welcome anyone’s help on that) > > It would be nice to have large message streamed in AMQP as we convert to > core right now. That’s the main thing I held a 2.7.0 release before. If I > can’t do it I will still do it. But if anyone is willing to help here it > would be awesome. > > > On Mon, Dec 17, 2018 at 11:57 AM Robbie Gemmell > wrote: > > > I'd say it has already been too long since the last one so I wouldnt > > wait until next year. A heads up is nice, and somewhat required when > > its been a long cycle and especially so if previous heads up have been > > missed. In this case I'd personally say it would be fine to proceed > > even later today if it seems ready and noone specifically objects, so > > it is out before people start to disappear for holidays. Other > > versions numbers are still available if anyone needs to do another > > release after. > > > > Robbie > > > > On Mon, 17 Dec 2018 at 16:03, Clebert Suconic > > > wrote: > > > > > > Would be too bad if I cut this tomorrow? So we have time for a vote > > before > > > the Christmas week. Or anyone would rather have it next year ? > > > > > > I had promised few weeks back but i was busy with a big chunk of work > > > around performance on AMQP. > > > -- > > > Clebert Suconic > > > -- > Clebert Suconic >
[GitHub] activemq-artemis issue #2466: NO-JIRA The MQTT consumer reconnection caused ...
Github user onlyMIT commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 @michaelandrepearce JIRA has been created ï¼ https://issues.apache.org/jira/projects/ARTEMIS/issues/ARTEMIS-2206?filter=addedrecently Regarding automated testing, in addition to the simulation method that I said, do you have any better suggestions? ---
[GitHub] activemq-artemis issue #2466: NO-JIRA The MQTT consumer reconnection caused ...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2466 This looks like it needs a JIRA and some automated tests. ---
