Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support useTopologyForLoadBalancing on JMS ConnectionFactory urls #2286

Closed
wants to merge 1 commit into from

Conversation

mvtorres
Copy link

No description provided.

Change-Id: I71ec5be35f8df22ddf1718ca8cb7e8da3906ba4b
@michaelandrepearce
Copy link
Contributor

Test cases needed to cover new feature, also documentation needed to describe the feature for others

@mvtorres
Copy link
Author

mvtorres commented Sep 4, 2018

This is an existing feature, I just exposed it so its usable on urls of ConnectionFactory.

Existing documentation is at https://activemq.apache.org/artemis/docs/latest/clusters.html#client-side-load-balancing

Im trying to add a testcase/logic to org.apache.activemq.artemis.tests.integration.cluster.distribution.NettySymmetricClusterTest

which was added by the original pull request

b6b8fa4

but Im having trouble getting that to execute locally.

When I run

mvn -Ptests -DfailIfNoTests=false -Dtest=NettySymmetricClusterTest test

I'm getting

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 56.893 s
[INFO] Finished at: 2018-09-04T12:54:56-04:00
[INFO] Final Memory: 127M/1567M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile (default-testCompile) on project unit-tests: Compilation failure: Compilation failure:
[ERROR] /Users/mark/git/activemq-artemis/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java:[62,81] cannot access ActiveMQConnectionFactory
[ERROR] class file for ActiveMQConnectionFactory not found
[ERROR] /Users/mark/git/activemq-artemis/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java:[93,81] incompatible types: ActiveMQConnectionFactory cannot be converted to org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory
[ERROR] /Users/mark/git/activemq-artemis/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java:[94,82] incompatible types: ActiveMQConnectionFactory cannot be converted to org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory

Can you point me to the right direction...

Thanks

@clebertsuconic
Copy link
Contributor

I usually just use the IDE to run tests...

But you probably need to build the project before running the test?

@clebertsuconic
Copy link
Contributor

TBH: This currently works already. you don't need to expose the property.

The URI is used to create the serverLocator, and it will be set on it already.

@mvtorres
Copy link
Author

mvtorres commented Sep 4, 2018

When I was running on debug mode, it was actually hitting

org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory.ActiveMQConnectionFactory(boolean, TransportConfiguration...)

instead of

org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory.ActiveMQConnectionFactory(ServerLocator)

so useTopologyForLoadBalancing was not being set on the ServerLocator.

I was using the following to get the ConnectionFactory

Properties p = new Properties();	
p.put("java.naming.factory.initial","org.apache.activemq.artemis.jndi.ActiveMQInitialContextFactory");
p.put("connectionFactory.ConnectionFactory","tcp://localhost:61616?useTopologyForLoadBalancing=false");
InitialContext initialContext = new InitialContext(p);
ConnectionFactory cf = (ConnectionFactory) initialContext.lookup("ConnectionFactory");

@clebertsuconic
Copy link
Contributor

Why don't you just use cf = new ActiveMQConnectionFActory(uri); ?

@clebertsuconic
Copy link
Contributor

anyway.. if you add the property.. also add the getter.. as the externalize will it.

@mvtorres
Copy link
Author

mvtorres commented Sep 4, 2018

Why don't you just use cf = new ActiveMQConnectionFActory(uri); ?

The existing code has the ConnectionFactory lookup via jndi, we're migrating from a different JMS provider(JBoss Messaging).

@jbertram
Copy link
Contributor

jbertram commented Sep 4, 2018

@mvtorres, how is JNDI being configured?

@mvtorres
Copy link
Author

mvtorres commented Sep 4, 2018

This is the config, our app is currently running on jboss 4.3.

<mbean code="org.jboss.naming.ExternalContext"
       name="DefaultDomain:service=ExternalContext,jndiName=RemoteJMSJNDI">
	  <attribute name="JndiName">RemoteJMSJNDI</attribute>
	  <attribute name="Properties">
	   		java.naming.factory.initial=org.apache.activemq.artemis.jndi.ActiveMQInitialContextFactory
		    java.naming.provider.url=tcp://localhost:61616?useTopologyForLoadBalancing=false
		    topic.topic/eventTopic=eventTopic
...
	  </attribute>
	  <attribute name="InitialContext">javax.naming.InitialContext</attribute>
</mbean>

Artemis is running inside docker, with the connector dynamically set to the docker container ip.

@jbertram
Copy link
Contributor

jbertram commented Sep 4, 2018

OK. I see now where this isn't being passed through when used for a JMS connection factory, although using the property in the URL for a ServerLocator directly does work. You should add a test to SimpleJNDIClientTest, e.g.:

   @Test
   public void testUseTopologyForLoadBalancing() throws NamingException {
      Hashtable<String, String> props = new Hashtable<>();
      props.put(Context.INITIAL_CONTEXT_FACTORY, "org.apache.activemq.artemis.jndi.ActiveMQInitialContextFactory");
      props.put("connectionFactory.ConnectionFactory", "vm://0?useTopologyForLoadBalancing=false");
      Context ctx = new InitialContext(props);

      ConnectionFactory connectionFactory = (ConnectionFactory) ctx.lookup("ConnectionFactory");
      assertFalse(((ActiveMQConnectionFactory)connectionFactory).getServerLocator().getUseTopologyForLoadBalancing());
   }

@clebertsuconic
Copy link
Contributor

@jbertram I think the right fix is on the Schemas...

   @Override
   protected ActiveMQConnectionFactory internalNewObject(URI uri,
                                                         Map<String, String> query,
                                                         String name) throws Exception {
      JMSConnectionOptions options = newConectionOptions(uri, query);
      ActiveMQConnectionFactory factory = ActiveMQJMSClient.createConnectionFactoryWithoutHA(options.getFactoryTypeEnum(), InVMTransportConfigurationSchema.createTransportConfiguration(uri, query, name, "org.apache.activemq.artemis.core.remoting.impl.invm.InVMConnectorFactory"));
      BeanSupport.setData(uri, factory, query);
      BeanSupport.setData(uri, factory.getServerLocator(), query);
      return factory;
   }

@clebertsuconic
Copy link
Contributor

Please look at #2292

clebertsuconic added a commit to clebertsuconic/activemq-artemis that referenced this pull request Sep 4, 2018
@mvtorres
Copy link
Author

mvtorres commented Sep 4, 2018

Thanks for the fix.

@asfgit asfgit closed this in d2d9a0e Sep 11, 2018
asfgit pushed a commit that referenced this pull request Sep 12, 2018
This closes #2286
As it superceedes it

(cherry picked from commit d2d9a0e)
franz1981 pushed a commit to franz1981/activemq-artemis that referenced this pull request Sep 12, 2018
franz1981 pushed a commit to franz1981/activemq-artemis that referenced this pull request Sep 18, 2018
This closes apache#2286
As it superceedes it

(cherry picked from commit d2d9a0e)
(cherry picked from commit 1ff80a8)
clebertsuconic added a commit to clebertsuconic/activemq-artemis that referenced this pull request Sep 19, 2018
k-wall pushed a commit to k-wall/activemq-artemis that referenced this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants