feat: new config to control constants in JMSWorker #141

Joel-hanson · 2024-11-22T11:49:56Z

feat: new config to control constants in JMSWorker

Description

New config for JMS receive timeout
New config for JMS reconnect minimum delay
New config for JMS reconnect maximum delay

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

dalelane

Thanks very much for picking this up - I've got a few minor comments, but functionally this is looking great

dalelane · 2024-11-25T10:27:56Z

.github/ISSUE_TEMPLATE/BUG-REPORT.yml

@@ -57,7 +57,7 @@ body:
      label: Version
      description: What version of our software are you running?
      options:
-        - 2.2.0 (Default)
+        - 2.2.1 (Default)


Suggested change

- 2.2.1 (Default)

- 2.3.0 (Default)

IMO, when doing semver, the patch number is for bug fixes, but new (compatible) features should increment the minor number.

README.md

dalelane · 2024-11-25T10:41:51Z

pom.xml

@@ -20,7 +20,7 @@
    <groupId>com.ibm.eventstreams.connect</groupId>
    <artifactId>kafka-connect-mq-source</artifactId>
    <packaging>jar</packaging>
-    <version>2.2.0</version>
+    <version>2.2.1</version>


Suggested change

<version>2.2.1</version>

<version>2.3.0</version>

rationale as above

dalelane · 2024-11-25T10:45:27Z

README.md

@@ -305,6 +305,9 @@ The configuration options for the Kafka Connect source connector for IBM MQ are
 | mq.message.mqmd.read                    | Whether to enable reading of all MQMD fields                                                                            | boolean | false          |                                                                                                                                                                                     |
 | mq.max.poll.blocked.time.ms             | How long the connector will wait for the previous batch of messages to be delivered to Kafka before starting a new poll | integer | 2000           | It is important that this is less than the time defined for `task.shutdown.graceful.timeout.ms` as that is how long connect will wait for the task to perform lifecycle operations. |
 | mq.client.reconnect.options             | Options governing MQ reconnection.                                                                                      | string  | ASDEF          | ASDEF, ANY, QMGR, DISABLED                                                                                                                                                          |
+| mq.jms.receive.timeout                  | The timeout in milliseconds for receiving messages from JMS Consumer.                                                   | long    | 2000L          | 1 or greater                                                                                                                                                                        |
+| mq.jms.reconnect.delay.min.ms           | The minimum delay in milliseconds for reconnect attempts.                                                               | long    | 64L            | 1 or greater                                                                                                                                                                        |


I think there is a risk that users will misunderstand this and confuse it with the JMS client reconnect feature - https://www.ibm.com/docs/en/ibm-mq/9.2?topic=reconnection-using-automatic-jms-client

It's unfortunate, because the name you've used is absolutely the most obvious and logical name. But I think we should name it differently to make it clear that this is how long the connector will pause after an exception before it tries again.

(Also, I recommend removing the "L" from the default value, as above)

dalelane · 2024-11-25T10:45:37Z

README.md

@@ -305,6 +305,9 @@ The configuration options for the Kafka Connect source connector for IBM MQ are
 | mq.message.mqmd.read                    | Whether to enable reading of all MQMD fields                                                                            | boolean | false          |                                                                                                                                                                                     |
 | mq.max.poll.blocked.time.ms             | How long the connector will wait for the previous batch of messages to be delivered to Kafka before starting a new poll | integer | 2000           | It is important that this is less than the time defined for `task.shutdown.graceful.timeout.ms` as that is how long connect will wait for the task to perform lifecycle operations. |
 | mq.client.reconnect.options             | Options governing MQ reconnection.                                                                                      | string  | ASDEF          | ASDEF, ANY, QMGR, DISABLED                                                                                                                                                          |
+| mq.jms.receive.timeout                  | The timeout in milliseconds for receiving messages from JMS Consumer.                                                   | long    | 2000L          | 1 or greater                                                                                                                                                                        |
+| mq.jms.reconnect.delay.min.ms           | The minimum delay in milliseconds for reconnect attempts.                                                               | long    | 64L            | 1 or greater                                                                                                                                                                        |
+| mq.jms.reconnect.delay.max.ms           | The maximum delay in milliseconds for reconnect attempts.                                                               | long    | 8192L          | 1 or greater                                                                                                                                                                        |


(same comment as for the min value above)

dalelane · 2024-11-25T10:45:55Z

src/main/java/com/ibm/eventstreams/connect/mqsource/MQSourceConnector.java

@@ -175,7 +190,7 @@ public class MQSourceConnector extends SourceConnector {
        CONFIG_VALUE_MQ_CLIENT_RECONNECT_OPTION_DISABLED.toLowerCase(Locale.ENGLISH)
    };

-    public static String version = "2.2.0";
+    public static String version = "2.2.1";


Suggested change

public static String version = "2.2.1";

public static String version = "2.3.0";

as above

dalelane · 2024-11-25T10:46:55Z

src/main/java/com/ibm/eventstreams/connect/mqsource/MQSourceConnector.java

@@ -531,6 +546,30 @@ null, new ReadableFile(),
                CONFIG_GROUP_MQ, 25,
                Width.SHORT,
                CONFIG_DISPLAY_MQ_CLIENT_RECONNECT_OPTIONS);
+        CONFIGDEF.define(CONFIG_MAX_RECEIVE_TIMEOUT,
+                Type.LONG,
+                CONFIG_MAX_RECEIVE_TIMEOUT_DEFAULT, ConfigDef.Range.atLeast(0),


This is a different minimum to what you've specified in the README.

Did you mean to allow the value 0?

dalelane · 2024-11-25T10:46:59Z

src/main/java/com/ibm/eventstreams/connect/mqsource/MQSourceConnector.java

+                CONFIG_DISPLAY_MAX_RECEIVE_TIMEOUT);
+        CONFIGDEF.define(CONFIG_RECONNECT_DELAY_MIN,
+                Type.LONG,
+                CONFIG_RECONNECT_DELAY_MIN_DEFAULT, ConfigDef.Range.atLeast(0),


This is a different minimum to what you've specified in the README.

Did you mean to allow the value 0?

dalelane · 2024-11-25T10:47:02Z

src/main/java/com/ibm/eventstreams/connect/mqsource/MQSourceConnector.java

+                CONFIG_DISPLAY_RECONNECT_DELAY_MIN);
+        CONFIGDEF.define(CONFIG_RECONNECT_DELAY_MAX,
+                Type.LONG,
+                CONFIG_RECONNECT_DELAY_MAX_DEFAULT, ConfigDef.Range.atLeast(0),


This is a different minimum to what you've specified in the README.

Did you mean to allow the value 0?

README.md

- New config for JMS receive timeout - New config for JMS reconnect minimum delay - New config for JMS reconnect maximum delay Signed-off-by: Joel Hanson <[email protected]>

Joel-hanson requested review from dalelane and priyankaku November 25, 2024 05:24

dalelane requested changes Nov 25, 2024

View reviewed changes

Joel-hanson commented Nov 25, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Joel-hanson commented Nov 25, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

feat: new config to control constants in JMSWorker

be56af6

- New config for JMS receive timeout - New config for JMS reconnect minimum delay - New config for JMS reconnect maximum delay Signed-off-by: Joel Hanson <[email protected]>

Joel-hanson force-pushed the fix-jms-work-with-custom-consumer-and-reconnect-values branch from b5c0a4a to be56af6 Compare November 29, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new config to control constants in JMSWorker #141

feat: new config to control constants in JMSWorker #141

Joel-hanson commented Nov 22, 2024 •

edited

Loading

dalelane left a comment

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

dalelane Nov 25, 2024

	public static String version = "2.2.1";
	public static String version = "2.3.0";

feat: new config to control constants in JMSWorker #141

Are you sure you want to change the base?

feat: new config to control constants in JMSWorker #141

Conversation

Joel-hanson commented Nov 22, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist

dalelane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joel-hanson commented Nov 22, 2024 •

edited

Loading