You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2016/04/18 17:46:06 UTC
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/469
ARTEMIS-488 Fix OpenWire Test (Temp Queue removal and others)
Temp Queue not deleted when connection is closed.
Enable Stomp in openwire test because some test uses it.
Remove unused code in opwnwire
Wrong XA error code returned when xid is missing
(ActiveMQXAConnectionFactory.testRollbackXaErrorCode)
regression in ActiveMQSslConnectionFactoryTest (SSL related)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis apache_master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/469.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 #469
----
commit 0ea6b712feec883091e22ef1a233bfb696f9ac05
Author: Howard Gao <ho...@gmail.com>
Date: 2016-04-18T15:39:13Z
ARTEMIS-488 Fix OpenWire Test (Temp Queue removal and others)
Temp Queue not deleted when connection is closed.
Enable Stomp in openwire test because some test uses it.
Remove unused code in opwnwire
Wrong XA error code returned when xid is missing
(ActiveMQXAConnectionFactory.testRollbackXaErrorCode)
regression in ActiveMQSslConnectionFactoryTest (SSL related)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212419562
@clebertsuconic can you ack the changes please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212073061
This is much better....
I'm not sure I like the SendResult. I would rather add a parameter to have the send throwing an error if no bindings. Much simpler change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212500301
The ProtocolManagerFactory still present on your PR.
I'm going to remove it myself during the merge. Please review it after merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/469#discussion_r60095912
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -361,6 +362,13 @@ public void run() {
connection.getTransportConnection().setAutoRead(false);
}
+ QueueQueryResult result = getCoreSession().executeQueueQuery(addresses[i]);
--- End diff --
We can change ServerSessionImpl::send as:
send(final ServerMessage message, final boolean direct, final boolean expectedRouting) throws Exception
we can down the road, change PostOfficeImpl::route to add expectedRouting parameter, and if the routeContext.getCount() == 0 and expectedRouting == true, then throw the exception.
Better than run this expensive query through the bindings on every message sent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/469#discussion_r60292159
--- Diff: tests/activemq5-unit-tests/src/main/resources/stomp/META-INF/services/org.apache.activemq.artemis.spi.core.protocol.ProtocolManagerFactory ---
@@ -0,0 +1 @@
+org.apache.activemq.artemis.core.protocol.stomp.StompProtocolManagerFactory
--- End diff --
Adding this file under testsuite would be a hack.
The problem with that is if we ever make any changes, or need any fix.. nobody will figure out what happened here.
Please remove this file from the testsuite, and if you need stomp protocol manager just add the proper dependency through the pom.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212440973
@mtaylor you are right. At first I thought those are very small fixes so it doesn't matter if I make one commit. But it turns out to have evolved into a big commit.
@clebertsuconic how about I split it into two commit?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212206653
I'll do the changes. Thanks Clebert.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212277797
hmm, some test failures. looks like not related to my changes...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-211698204
Thanks clebert. I think you are right. There is an internal core session that can be used to created queues. I'm going to try use that. I'll think of how to improve the check queue deletion code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-211472570
Howard,
You can't simply duplicate functionality that is already available on the Broker.
ServerSessionImpl, which is used by OpenWireConnection already has controller for temporary queues. Why we can't simply use the TemporaryQueues from The ServerSession?
I don't want to keep adding copy & paste inheritance on the openwire protocol head. if something on the broker is not working as expected I would rather change with parameter to adapt any semantics than copy & paste and duplicate functionality just to fix tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212502990
lets do it for next PRs.
The idea is that you keep concentrate within issues.. but you still rebase and squash accordingly.
I usually have lots of small committs on my branch and then I squash them accordingly to atomic issues before PR.
An example of a previous PR I worked on:
https://github.com/apache/activemq-artemis/pull/463
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212432383
@howardgao Just looking at this, wanted just to say it's usually good practice to keep the commits within reasonably small scope. This makes it easier to understand what the code changes are trying to achieve, for review or when looking at the history. It looks like you're fixing about 5 issues in one huge commit here, would probably be best to have 1 commit per issue/fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-211480175
The proper change for the Temporary Queue would be to call internalSession.createQueue instead of duplicating the createQueue control through the protocol head.
So, that makes it two changes to the PR:
i - adapt the create queue, and make sure the ServerSession close is called when things happens (I think it is).
ii - change the routing to add a new overloaded method to be used by OpenWire instead of queueQuery on every message. (if that even is required... we need to check if that's really needed... because if the temp queue was deleted.. it simply means we don't need to bother.. just relax the test).
You may ping me before you do these changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/469#discussion_r60095317
--- Diff: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java ---
@@ -361,6 +362,13 @@ public void run() {
connection.getTransportConnection().setAutoRead(false);
}
+ QueueQueryResult result = getCoreSession().executeQueueQuery(addresses[i]);
--- End diff --
Really?
This would really suck in performance... it's not a good thing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212241654
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-212081809
Ok, I like the SendResult idea after all. With just a few modifications to make it really cool and a consistent API.
- Rename SendResult as RoutingStatus (or find a better name, being part of Routing)
- Move it under org.apache.activemq.artemis.core.postoffice
- The enum constants should be. (No need to set any prefix on them).
OK, NO_BINDINGS, NO_BINDINGS_DLA, DUPLICATED_ID
- Notice that there's no need for SEND_NO_DLA API wise.. just make it NO_BINDINGS.
- Also, make all the PostOffice::route operations to return RoutingStatus. to make the API clean.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/469
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request: ARTEMIS-488 Fix OpenWire Test (Temp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the pull request:
https://github.com/apache/activemq-artemis/pull/469#issuecomment-211998627
Hi Clebert, Just updated my branch with new changes. I'm adding a return value to session.send(), which is a enum called SendResult to help check the temp queues.
Please review the code when you have time.
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---