You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/02/09 22:56:21 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request #2976:
ARTEMIS-2325 + a few other fixes
jbertram opened a new pull request #2976: ARTEMIS-2325 + a few other fixes
URL: https://github.com/apache/activemq-artemis/pull/2976
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request
#2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377905626
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientProducerImpl.java
##########
@@ -138,24 +135,19 @@ public void send(SimpleString address1,
Message message,
SendAcknowledgementHandler handler) throws ActiveMQException {
checkClosed();
- boolean confirmationWindowEnabled = session.isConfirmationWindowEnabled();
- if (confirmationWindowEnabled) {
- doSend(address1, message, handler);
- } else {
- doSend(address1, message, null);
- if (handler != null) {
- if (logger.isDebugEnabled()) {
- logger.debug("Handler was used on producing messages towards address " + (address1 == null ? null : address1.toString()) + " however there is no confirmationWindowEnabled");
- }
+ if (handler != null) {
+ handler = new SendAcknowledgementHandlerWrapper(handler);
Review comment:
No, it won't create a new object for every message send. It will only create a wrapper for the handler if a handler is set when a message is sent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377930173
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/SendAcknowledgementHandlerWrapper.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.core.client.impl;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.client.SendAcknowledgementHandler;
+
+public class SendAcknowledgementHandlerWrapper implements SendAcknowledgementHandler {
+
+ private SendAcknowledgementHandler wrapped;
+
+ /**
+ * It's possible that a SendAcknowledgementHandler might be called twice due to subsequent
+ * packet confirmations on the same connection. Using this boolean avoids that possibility.
+ * A new SendAcknowledgementHandlerWrapper is created for each message sent so once it's
+ * called once it will never be called again.
+ */
+ private AtomicBoolean active = new AtomicBoolean(true);
Review comment:
fair enough.. can you at least use a regular volatile boolean here?
the sendAck is called from a single thread. I think it's safe to use a regular boolean here.
if you really want Atomics used, can you use a AtomicIntegerFieldUpdater instead? have a counter and only call this if the counter == 0. AtomicBoolean will be heavier then a regular field.
That's the only change I will ask, on top of the favour I asked you. (removing the XXX)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #2976: ARTEMIS-2325 +
a test fix
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request
#2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r379075167
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/SendAcknowledgementHandlerWrapper.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.core.client.impl;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.client.SendAcknowledgementHandler;
+
+public class SendAcknowledgementHandlerWrapper implements SendAcknowledgementHandler {
+
+ private SendAcknowledgementHandler wrapped;
+
+ /**
+ * It's possible that a SendAcknowledgementHandler might be called twice due to subsequent
+ * packet confirmations on the same connection. Using this boolean avoids that possibility.
+ * A new SendAcknowledgementHandlerWrapper is created for each message sent so once it's
+ * called once it will never be called again.
+ */
+ private AtomicBoolean active = new AtomicBoolean(true);
Review comment:
Scratch that. My non-wrapper solution had compatibility issues. :(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request
#2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r379001372
##########
File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##########
@@ -534,62 +524,56 @@ private CompletionListenerWrapper(CompletionListener listener,
@Override
public void sendAcknowledged(org.apache.activemq.artemis.api.core.Message clientMessage) {
- if (active.get()) {
- if (jmsMessage instanceof StreamMessage) {
- try {
- ((StreamMessage) jmsMessage).reset();
- } catch (JMSException e) {
- // HORNETQ-1209 XXX ignore?
- }
- }
- if (jmsMessage instanceof BytesMessage) {
- try {
- ((BytesMessage) jmsMessage).reset();
- } catch (JMSException e) {
- // HORNETQ-1209 XXX ignore?
- }
+ if (jmsMessage instanceof StreamMessage) {
+ try {
+ ((StreamMessage) jmsMessage).reset();
+ } catch (JMSException e) {
+ // HORNETQ-1209 XXX ignore?
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377897408
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/SendAcknowledgementHandlerWrapper.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.core.client.impl;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.client.SendAcknowledgementHandler;
+
+public class SendAcknowledgementHandlerWrapper implements SendAcknowledgementHandler {
Review comment:
is this for every message sent?
if you need such thing I would rather add something to the ClientMessage itself instead of creating a new object.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377929374
##########
File path: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessageProducer.java
##########
@@ -534,62 +524,56 @@ private CompletionListenerWrapper(CompletionListener listener,
@Override
public void sendAcknowledged(org.apache.activemq.artemis.api.core.Message clientMessage) {
- if (active.get()) {
- if (jmsMessage instanceof StreamMessage) {
- try {
- ((StreamMessage) jmsMessage).reset();
- } catch (JMSException e) {
- // HORNETQ-1209 XXX ignore?
- }
- }
- if (jmsMessage instanceof BytesMessage) {
- try {
- ((BytesMessage) jmsMessage).reset();
- } catch (JMSException e) {
- // HORNETQ-1209 XXX ignore?
- }
+ if (jmsMessage instanceof StreamMessage) {
+ try {
+ ((StreamMessage) jmsMessage).reset();
+ } catch (JMSException e) {
+ // HORNETQ-1209 XXX ignore?
Review comment:
not related to your change but can you please remove this XXX?
This is mark I used to add on code.
I don't recall if I did this, but this is a thing I used to do to mark the code with stuff like During a big refactoring I would mark XXX, and remove it before merging.. so it was intended to be removed from 10 years ago :)
you could replace this thing by just logger.debug
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request
#2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377905936
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/SendAcknowledgementHandlerWrapper.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.core.client.impl;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.client.SendAcknowledgementHandler;
+
+public class SendAcknowledgementHandlerWrapper implements SendAcknowledgementHandler {
Review comment:
No, it isn't for every message sent. It's only used when a handler is set (either on the session or passed-in when sent).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] jbertram commented on a change in pull request
#2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r379001542
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/SendAcknowledgementHandlerWrapper.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.core.client.impl;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.client.SendAcknowledgementHandler;
+
+public class SendAcknowledgementHandlerWrapper implements SendAcknowledgementHandler {
+
+ private SendAcknowledgementHandler wrapped;
+
+ /**
+ * It's possible that a SendAcknowledgementHandler might be called twice due to subsequent
+ * packet confirmations on the same connection. Using this boolean avoids that possibility.
+ * A new SendAcknowledgementHandlerWrapper is created for each message sent so once it's
+ * called once it will never be called again.
+ */
+ private AtomicBoolean active = new AtomicBoolean(true);
Review comment:
I've reworked this a bit to avoid the wrapper altogether.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2976: ARTEMIS-2325 + a test fix
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2976: ARTEMIS-2325 + a test fix
URL: https://github.com/apache/activemq-artemis/pull/2976#discussion_r377896828
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientProducerImpl.java
##########
@@ -138,24 +135,19 @@ public void send(SimpleString address1,
Message message,
SendAcknowledgementHandler handler) throws ActiveMQException {
checkClosed();
- boolean confirmationWindowEnabled = session.isConfirmationWindowEnabled();
- if (confirmationWindowEnabled) {
- doSend(address1, message, handler);
- } else {
- doSend(address1, message, null);
- if (handler != null) {
- if (logger.isDebugEnabled()) {
- logger.debug("Handler was used on producing messages towards address " + (address1 == null ? null : address1.toString()) + " however there is no confirmationWindowEnabled");
- }
+ if (handler != null) {
+ handler = new SendAcknowledgementHandlerWrapper(handler);
Review comment:
this will create a new object for every message send called when no handlers in place?
that's a heavy change, no?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services