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

Added MQ Connection Options to allow configuring of connection programatically #55

Merged

Conversation

azarc-jono-langley
Copy link
Contributor

@azarc-jono-langley azarc-jono-langley commented Oct 28, 2022

Added the ability to provide config options on context creation. This allows configuration of channel definitions outside of the connection configuration and provides a way to programatically provide options without the need to have custom Connection factories.

Example of use:

factory = mqjms.ConnectionFactoryImpl{...}

ctx, jmsErr := factory.CreateContext(
      // Options helper
      mqjms.WithMaxMsgLength(65536),

      // Custom options
      func(cno *mqjms.MQCNO) {
            cno.ClientConn.KeepAliveInterval = 160 
      },
)

I accept the terms of the Contributor License Agreement.

@matrober-uk
Copy link
Member

Hi @azarc-jono-langley, thanks for your PR - I'll take a look and share comments over the next week or so.

In the mean time, please can you confirm your agreement to the Contributor License Agreement per our process for Contributing, thanks!

Contributions to this package must be made under the terms of the IBM Contributor License Agreement, found in the [CLA file](https://github.com/ibm-messaging/mq-golang-jms20/blob/main/CLA.md) of this repository. When submitting a pull request, you must include a statement stating you accept the terms in the CLA.

@matrober-uk
Copy link
Member

matrober-uk commented Oct 31, 2022

Hi @azarc-jono-langley - I tried out your PR, and identified three areas that need some additional work please before we can merge to main.

1. Backwards compatibility
Running the existing test cases confirmed what I suspected which is that as currently written the change breaks backwards compatibility for any existing applications because it removes the existing CreateContext() function signature that they make use of.

go test -v
# github.com/ibm-messaging/mq-golang-jms20 [github.com/ibm-messaging/mq-golang-jms20.test]
./requestreply_test.go:80:20: cannot use cf (variable of type mqjms.ConnectionFactoryImpl) as type jms20subset.ConnectionFactory in argument to replyToMessage:
	mqjms.ConnectionFactoryImpl does not implement jms20subset.ConnectionFactory (wrong type for CreateContext method)
		have CreateContext(mqos ...mqjms.MQOptions) (jms20subset.JMSContext, jms20subset.JMSException)
		want CreateContext() (jms20subset.JMSContext, jms20subset.JMSException)
FAIL	github.com/ibm-messaging/mq-golang-jms20 [build failed]

It's straightforward to avoid that side effect though - please could you refactor the request as follows;

  1. Retain the existing CreateContext() function signature
  2. Add a function signature CreateContext(MQOptions)
  3. Change the existing implementation of CreateContext so that it accepts the MQOptions parameter (same as you've done in the PR)
  4. Create a new implementation for CreateContext() function signature, and have it delegate to the MQOptions variant of the call with an empty/nil parameter to indicate the user didn't provide anything - so that there is only one implementation function that does the real work, and the simpler one is a trivial facade into it, without the extra parameter

(and the equivalent pattern for CreateContextWithSessionMode(int)

We should then be in the position where existing applications (and test cases) can execute successfully without modification, and any applications that want to use the new MQOptions variation of the function to get the more advanced behaviour.

2. Path coverage for tests
Additionally, we should also have a test case that covers the CreateContextWithSessionMode path please.

3. Functional behaviour of tests
Can we test at least one of the CNO properties "functionally" in the test cases to confirm that it is applied correctly at runtime? For example if you set the MaxMsgLength very short (much shorter than the default value) and then tried to send a message longer than that length it should trigger an appropriate error message that we can check for?

Copy link
Member

@matrober-uk matrober-uk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three areas of update request in #55 (comment)

  • Backwards compatibility
  • Path coverage of tests
  • Functional behabiour of tests

@azarc-jono-langley
Copy link
Contributor Author

requestreply_test.go:

Hi @matrober-uk

Thanks for feedback. Apologies for the miss, the unfortunate side affect of using ARM for my development machine means working with IBMMQ client libraries difficult :)

In line with your above suggestions:

  1. Is your suggestion to add a new signature to the interface jms20subset.ConnectionFactory or only on the mqjms.ConnectionFactoryImpl? Just want to make sure i understand you correctly.

  2. There is a test for CreateContext (TestMQConnectionOptions), shall i create one for CreateContextWithSessionMode too? Internally CreateContext calls CreateContextWithSessionMode with default values but i can add both if required.

  3. I can add that if required, we have a test to validate that the value was set on the connection (for that example) but I assumed somewhere that connection property is being tested as this property could be set via the config files. Happy to add if required.

  4. Licence agreement, added.

@matrober-uk
Copy link
Member

requestreply_test.go:

Hi @matrober-uk

Thanks for feedback. Apologies for the miss, the unfortunate side affect of using ARM for my development machine means working with IBMMQ client libraries difficult :)

In line with your above suggestions:

1. Is your suggestion to add a new signature to the interface `jms20subset.ConnectionFactory` or only on the `mqjms.ConnectionFactoryImpl`? Just want to make sure i understand you correctly.

2. There is a test for `CreateContext` (`TestMQConnectionOptions`), shall i create one for `CreateContextWithSessionMode` too? Internally `CreateContext` calls `CreateContextWithSessionMode` with default values but i can add both if required.

3. I can add that if required, we have a test to validate that the value was set on the connection (for that example) but I assumed somewhere that connection property is being tested as this property could be set via the config files. Happy to add if required.

4. Licence agreement, added.

Hi @azarc-jono-langley - no problem, I appreciate you taking the time to submit a PR in the first place!

To your questions:

  1. Yes, we should add both on the ConnectionFactory, and also the matching implementation on ConnectionFactoryImpl
  2. Yes please, I'd like to maintain test cases for each of the different code paths to maximise the likelihood of catching accidental introduction of bugs if/when the internal implementation might change in the future
  3. Where possible it's good to test "functionally" that the value being set is taking effect at runtime, so something like the message length check I suggested would be good please
  4. thanks for adding your confirmation of agreement to the CLA

@azarc-jono-langley azarc-jono-langley force-pushed the feature/add-context-creation-options branch 2 times, most recently from 15f0f50 to 27f7e01 Compare November 2, 2022 21:36
@azarc-jono-langley azarc-jono-langley force-pushed the feature/add-context-creation-options branch from 27f7e01 to 44b5720 Compare November 3, 2022 14:15
@azarc-jono-langley
Copy link
Contributor Author

requestreply_test.go:

Hi @matrober-uk
Thanks for feedback. Apologies for the miss, the unfortunate side affect of using ARM for my development machine means working with IBMMQ client libraries difficult :)
In line with your above suggestions:

1. Is your suggestion to add a new signature to the interface `jms20subset.ConnectionFactory` or only on the `mqjms.ConnectionFactoryImpl`? Just want to make sure i understand you correctly.

2. There is a test for `CreateContext` (`TestMQConnectionOptions`), shall i create one for `CreateContextWithSessionMode` too? Internally `CreateContext` calls `CreateContextWithSessionMode` with default values but i can add both if required.

3. I can add that if required, we have a test to validate that the value was set on the connection (for that example) but I assumed somewhere that connection property is being tested as this property could be set via the config files. Happy to add if required.

4. Licence agreement, added.

Hi @azarc-jono-langley - no problem, I appreciate you taking the time to submit a PR in the first place!

To your questions:

  1. Yes, we should add both on the ConnectionFactory, and also the matching implementation on ConnectionFactoryImpl
  2. Yes please, I'd like to maintain test cases for each of the different code paths to maximise the likelihood of catching accidental introduction of bugs if/when the internal implementation might change in the future
  3. Where possible it's good to test "functionally" that the value being set is taking effect at runtime, so something like the message length check I suggested would be good please
  4. thanks for adding your confirmation of agreement to the CLA

Ok, I've made the changes now.

  1. Changed the signature to have options for both CreateContext and CreateContextWithSessionMode
  2. Added tests for checking the values are set by options definitions
  3. E2E test created MaxMsgLength is respected when receive message on CreateContextWithSessionMode
  4. Done

One thing to note, when sending messages over the size limit, it looks like the send request hangs when sending to the message queue. It must be something to do with the underlying IBMMQ Client lib, i didn't have time to investigate so i implemented a send context which could send the message to the queue but on the receive from queue with a small MaxMsgLength configured, the lib throws the expected error MQRC_DATA_LENGTH_ERROR.

Copy link
Member

@matrober-uk matrober-uk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @azarc-jono-langley for making the updates I requested, looks good!

there is one minor addition that I'll fix under a separate PR - to consume the message that is sent by the testcase at the end so that the queue is left clean for the next test in the suite, but I'll handle that update.

@matrober-uk matrober-uk merged commit c12f9a5 into ibm-messaging:main Nov 5, 2022
matrober-uk added a commit to matrober-uk/mq-golang-jms20 that referenced this pull request Nov 5, 2022
matrober-uk added a commit to matrober-uk/mq-golang-jms20 that referenced this pull request Nov 5, 2022
matrober-uk added a commit that referenced this pull request Nov 5, 2022
Clean up message after conn options test - #55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants