[GitHub] [activemq-nms-amqp] cjwmorgan-sol commented on pull request #56: AMQNET-637 NMS 2.0 - Add NMS 2.0 method implementations.

[GitBox <gi...@apache.org>]

cjwmorgan-sol commented on pull request #56:
URL: https://github.com/apache/activemq-nms-amqp/pull/56#issuecomment-648370488


   The only concerns I have is for the order of execution of the async send and application network send vs send result notifications. 
   
   This might just be my naiveté towards the C# async patterns. Wouldn't the async sends potentially get out of order as the task executor would be on a thread pool that is system default based? I do not see how the send tasks execution order is synchronized internal in the provider to ensure send order from the application thread. Perhaps I'm missing something? This might also be a concept for the completion result signal, sounds like above the async implementation might deviate a bit from JMS spec perhaps I'm missing some rationale behind that as well. Right now my view on a NMS async send would be the nio equivalent (or something close to) of the JMS 2.0 send.
   
   Also the async send seems to produce different tasks depending on delivery mode (persistent vs non-persistent) having the task complete on network io complete for non-persistent and outcome result for persistent. This also leaves the application is a state where its unware of the messages send state. Although thinking about it with amqpnetlite "send" might mean buffered instead of io complete. However removing the capabilities of the application of separating network io complete vs response complete might be problematic back at the NMS api level should application require both network io complete and send outcome completion as separate events. Would such a  capability be out of scope for the NMS 2.0 improvements? Or is there some rationale I'm missing for the decide to combine the event of network io completion and send request completion?
   
   I also tried the unit tests using the NMS 2.0 api from @michaelpearce-gain pull request they all passed. Are there any tests for the ordering of messages sent and received I'd image so I have not found them. (I'm pretty sure they should be somewhere).
   I still need to try to update my testing tool a bit before I test any of the new significant changes done in NMS 2.0. Not sure when or if I'll be able to do that. I'll be relying heavily on testing in the meantime. :)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org