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 2021/07/07 09:23:56 UTC
[GitHub] [activemq-nms-amqp] lukeabsent opened a new pull request #68: Amqnet 637
lukeabsent opened a new pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce merged pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce merged pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r677808281
##########
File path: test/Apache-NMS-AMQP-Test/Integration/TransactionsIntegrationTest.cs
##########
@@ -997,6 +997,12 @@ public void TestSendAfterCoordinatorLinkClosedDuringTX()
testPeer.WaitForAllMatchersToComplete(2000);
+ // Make sure that above line 'WaitForAllMatchersToComplete' also finishes on session/producer side
+ // In order for producer.Send to not really send, transaction on this end needs to be IsTransactionFailed
+ // in AmqpProducer if (session.IsTransacted && session.IsTransactionFailed), and sometimes producer.Send tries
+ // to send before this flag is set, which would cause actual transfer
+ Thread.Sleep(10);
Review comment:
This is dubious........ anything thats a pause...sounds fishy.....
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r677809103
##########
File path: test/Apache-NMS-AMQP-Test/TestAmqp/TestAmqpPeerRunner.cs
##########
@@ -131,7 +130,7 @@ void Pump(Stream stream)
catch(Exception e)
{
Logger.Info(e);
- stream.Dispose();
+ // stream.Dispose();
Review comment:
commented code?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r678231699
##########
File path: test/Apache-NMS-AMQP-Interop-Test/NmsMessageConsumerTest.cs
##########
@@ -310,8 +313,9 @@ public void TestSharedDurableSubscription()
messageConsumer1.Close();
messageConsumer2.Close();
+
sessionConsumer1.Unsubscribe(subscriptionName);
- sessionConsumer2.Unsubscribe(subscriptionName);
+ // sessionConsumer2.Unsubscribe(subscriptionName); // seems that first unsubscribe removes the first subscription
Review comment:
@lukeabsent
##########
File path: test/Apache-NMS-AMQP-Test/Integration/TransactionsIntegrationTest.cs
##########
@@ -997,6 +997,12 @@ public void TestSendAfterCoordinatorLinkClosedDuringTX()
testPeer.WaitForAllMatchersToComplete(2000);
+ // Make sure that above line 'WaitForAllMatchersToComplete' also finishes on session/producer side
+ // In order for producer.Send to not really send, transaction on this end needs to be IsTransactionFailed
+ // in AmqpProducer if (session.IsTransacted && session.IsTransactionFailed), and sometimes producer.Send tries
+ // to send before this flag is set, which would cause actual transfer
+ Thread.Sleep(10);
Review comment:
@lukeabsent
##########
File path: test/Apache-NMS-AMQP-Test/TestAmqp/TestAmqpPeerRunner.cs
##########
@@ -131,7 +130,7 @@ void Pump(Stream stream)
catch(Exception e)
{
Logger.Info(e);
- stream.Dispose();
+ // stream.Dispose();
Review comment:
@lukeabsent
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelpearce-gain commented on pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#issuecomment-887295860
Can you separate out of this PR for the No-Jiras, PR for 718 and also squash the commits, with a more meaningful commit message.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#issuecomment-891949487
LGTM, great stuff thanks for fixing this up +100
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] lukeabsent commented on pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
lukeabsent commented on pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#issuecomment-879951558
:-)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r677807243
##########
File path: test/Apache-NMS-AMQP-Interop-Test/NmsMessageConsumerTest.cs
##########
@@ -310,8 +313,9 @@ public void TestSharedDurableSubscription()
messageConsumer1.Close();
messageConsumer2.Close();
+
sessionConsumer1.Unsubscribe(subscriptionName);
- sessionConsumer2.Unsubscribe(subscriptionName);
+ // sessionConsumer2.Unsubscribe(subscriptionName); // seems that first unsubscribe removes the first subscription
Review comment:
no need to leave commented lines of code in.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r677808281
##########
File path: test/Apache-NMS-AMQP-Test/Integration/TransactionsIntegrationTest.cs
##########
@@ -997,6 +997,12 @@ public void TestSendAfterCoordinatorLinkClosedDuringTX()
testPeer.WaitForAllMatchersToComplete(2000);
+ // Make sure that above line 'WaitForAllMatchersToComplete' also finishes on session/producer side
+ // In order for producer.Send to not really send, transaction on this end needs to be IsTransactionFailed
+ // in AmqpProducer if (session.IsTransacted && session.IsTransactionFailed), and sometimes producer.Send tries
+ // to send before this flag is set, which would cause actual transfer
+ Thread.Sleep(10);
Review comment:
This is dubious........ anything thats a pause of an arbitary value...sounds fishy.....
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] Havret commented on pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
Havret commented on pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#issuecomment-879684425
It's all green. Awesome. :)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-amqp] lukeabsent commented on a change in pull request #68: Amqnet 637
Posted by GitBox <gi...@apache.org>.
lukeabsent commented on a change in pull request #68:
URL: https://github.com/apache/activemq-nms-amqp/pull/68#discussion_r678448642
##########
File path: test/Apache-NMS-AMQP-Test/Integration/TransactionsIntegrationTest.cs
##########
@@ -997,6 +997,12 @@ public void TestSendAfterCoordinatorLinkClosedDuringTX()
testPeer.WaitForAllMatchersToComplete(2000);
+ // Make sure that above line 'WaitForAllMatchersToComplete' also finishes on session/producer side
+ // In order for producer.Send to not really send, transaction on this end needs to be IsTransactionFailed
+ // in AmqpProducer if (session.IsTransacted && session.IsTransactionFailed), and sometimes producer.Send tries
+ // to send before this flag is set, which would cause actual transfer
+ Thread.Sleep(10);
Review comment:
I dont see much options here, the way I see it for test to go expected way, that flag mentioned in comment should switch, but I dont see any nice way of checking/waiting for it here and signal : /
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org