date:20190107

[GitHub] ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for IBM MQ multi-instance queue managers

2019-01-07 Thread GitBox

ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for 
IBM MQ multi-instance queue managers
URL: https://github.com/apache/nifi/pull/3246#discussion_r245868656
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java
 ##
 @@ -185,23 +185,31 @@ public void disable() {
  * service configuration. For example, 'channel' property will correspond 
to
  * 'setChannel(..) method and 'queueManager' property will correspond to
  * setQueueManager(..) method with a single argument.
- * 
+ * 
 
 Review comment:
   Separated paragraphs can provide better readability for this long java doc. 
I'd leave the `` tag as is. Or adding more of it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for IBM MQ multi-instance queue managers

2019-01-07 Thread GitBox

ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for 
IBM MQ multi-instance queue managers
URL: https://github.com/apache/nifi/pull/3246#discussion_r245882407
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java
 ##
 @@ -210,17 +218,23 @@ private void 
setConnectionFactoryProperties(ConfigurationContext context) {
 if (descriptor.isDynamic()) {
 this.setProperty(propertyName, entry.getValue());
 } else {
-if (propertyName.equals(BROKER)) {
-String brokerValue = 
context.getProperty(descriptor).evaluateAttributeExpressions().getValue();
-if 
(context.getProperty(CONNECTION_FACTORY_IMPL).evaluateAttributeExpressions().getValue().startsWith("org.apache.activemq"))
 {
+if (descriptor == BROKER_URI) {
+String brokerValue = 
context.getProperty(BROKER_URI).evaluateAttributeExpressions().getValue();
+String connectionFactoryValue = 
context.getProperty(CONNECTION_FACTORY_IMPL).evaluateAttributeExpressions().getValue();
+if 
(connectionFactoryValue.startsWith("org.apache.activemq")) {
 this.setProperty("brokerURL", brokerValue);
+} else if 
(connectionFactoryValue.startsWith("com.tibco.tibjms")) {
+this.setProperty("serverUrl", brokerValue);
 } else {
+// Try to parse broker URI as colon separated 
host/port pair
 String[] hostPort = brokerValue.split(":");
+// If broker URI indeed was colon separated host/port 
pair
 if (hostPort.length == 2) {
 this.setProperty("hostName", hostPort[0]);
 this.setProperty("port", hostPort[1]);
-} else if (hostPort.length != 2) {
-this.setProperty("serverUrl", brokerValue); // for 
tibco
+} else if 
(connectionFactoryValue.startsWith("com.ibm.mq.jms")) {
+// Assuming IBM MQ style broker was specified, 
e.g. "myhost(1414)" and "myhost01(1414),myhost02(1414)"
+this.setProperty("connectionNameList", 
brokerValue);
 
 Review comment:
   After reading some articles and API specs for connectionNameList property, 
just wondering if setting `connectionNameList` by a dynamic property work. Did 
you try before working on this PR?
   
   What if both `host` and `port` pair and `connectionNameList` are set?
   
   > Specifies the hosts to which the client will attempt to reconnect to after 
its connection is broken.
   
https://www.ibm.com/support/knowledgecenter/en/SSFKSJ_7.1.0/com.ibm.mq.javadoc.doc/WMQJMSClasses/com/ibm/mq/jms/MQConnectionFactory.html#setConnectionNameList_java.lang.String_


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for IBM MQ multi-instance queue managers

2019-01-07 Thread GitBox

ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for 
IBM MQ multi-instance queue managers
URL: https://github.com/apache/nifi/pull/3246#discussion_r245879771
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/test/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProviderTest.java
 ##
 @@ -101,4 +101,157 @@ public void 
validateGetConnectionFactoryFailureIfServiceNotConfigured() throws E
 new JMSConnectionFactoryProvider().getConnectionFactory();
 }
 
+@Test
+public void validWithSingleTestBroker() throws Exception {
+TestRunner runner = TestRunners.newTestRunner(mock(Processor.class));
+
+JMSConnectionFactoryProvider cfProvider = new 
JMSConnectionFactoryProvider();
+runner.addControllerService("cfProvider", cfProvider);
+
+String clientLib = 
this.getClass().getResource("/dummy-lib.jar").toURI().toString();
+
+runner.setProperty(cfProvider, 
JMSConnectionFactoryProvider.BROKER_URI, "myhost:1234");
+runner.setProperty(cfProvider, 
JMSConnectionFactoryProvider.CLIENT_LIB_DIR_PATH, clientLib);
+runner.setProperty(cfProvider, 
JMSConnectionFactoryProvider.CONNECTION_FACTORY_IMPL,
+"org.apache.nifi.jms.testcflib.TestConnectionFactory");
+
+runner.assertValid(cfProvider);
 
 Review comment:
   Since JMSConnectionFactoryProvider doesn't implement `customValidate` method 
and the NonEmptyBrokerURIValidator used for validating the BROKER_URI property 
doesn't check connection factory impl value, these test variations do not 
assert what is expected.
   
   Alternative approach would be creating a sub-class of 
JMSConnectionFactoryProvider only for testing purpose which overrides 
`setProperty(String propertyName, Object propertyValue)` then do assertion 
within the overridden setProperty method. This way, we can make sure what 
property is set by combination of broker uri and connection factory impl.
   
   How do you think?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for IBM MQ multi-instance queue managers

2019-01-07 Thread GitBox

ijokarumawak commented on a change in pull request #3246: NIFI-5929 Support for
IBM MQ multi-instance queue managers
URL: https://github.com/apache/nifi/pull/3246#discussion_r245880875

##
File path:
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java
##
@@ -185,23 +185,31 @@ public void disable() {
* service configuration. For example, 'channel' property will correspond
to
* 'setChannel(..) method and 'queueManager' property will correspond to
* setQueueManager(..) method with a single argument.
- *
+ *
* There are also few adjustments to accommodate well known brokers. For
* example ActiveMQ ConnectionFactory accepts address of the Message Broker
* in a form of URL while IBMs in the form of host/port pair (more common).
* So this method will use value retrieved from the 'BROKER_URI' static
* property 'as is' if ConnectionFactory implementation is coming from
- * ActiveMQ and for all others (for now) the 'BROKER_URI' value will be
+ * ActiveMQ or Tibco. For all others (for now) the 'BROKER_URI' value will
be
* split on ':' and the resulting pair will be used to execute
* setHostName(..) and setPort(..) methods on the provided
- * ConnectionFactory. This may need to be maintained and adjusted to
- * accommodate other implementation of ConnectionFactory, but only for
- * URL/Host/Port issue. All other properties are set as dynamic properties
- * where user essentially provides both property name and value, The bean
- * convention is also explained in user manual for this component with
links
- * pointing to documentation of various ConnectionFactories.
+ * ConnectionFactory. An exception to this if the ConnectionFactory
+ * implementation is coming from IBM MQ and multiple brokers are listed,
+ * in this case setConnectionNameList(..) method is executed.
+ * This may need to be maintained and adjusted to accommodate other
+ * implementation of ConnectionFactory, but only for URL/Host/Port issue.
+ * All other properties are set as dynamic properties where user
essentially
+ * provides both property name and value, The bean convention is also
+ * explained in user manual for this component with links pointing to
+ * documentation of various ConnectionFactories.

Review comment:
Thanks for elaborating the Javadocs. Very helpful to understand how the same
property can be used differently based on the target MQ broker systems.

The sad thing is, these helpful information will not be seen by general NiFi
users from NiFi UI usage.

I think this is a good timing to adding more on 'Additional Details' page
for this component. As a user I'd like to see following information laid out
nicely in the additional doc page:

- Target MQ System: i.e. ActiveMQ, IBM, Tibco ... etc
- MQ ConnectionFactory Implementation
- Broker URI: Especially for the IBM case where single queue manager
instance and multiple ones.

We can link to external doc pages for each vender specific information from
there.

How do you think?

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.

62 matches

Mail list logo