gemmellr commented on issue #32: QPIDJMS-441: add proxy support URL: https://github.com/apache/qpid-jms/pull/32#issuecomment-546010698 As Tim and I suggested, a specific test class (e.g ProxyIntegrationTest) for the full-connection proxy tests would seem better (i.e not the options or transport tests). I don't think the connection related changes to IntegrationTestFixture and FailoverIntegration test are that desirable given the added methods aren't going to be used widely or at all in some cases. A util method for connecting within the new test class would do nicely for this. Tim noted to me just now that he remembered there is a websocket-capable basic test server in the client module these days, which I had forgotten, so you should be able to use that and thus remove the tests and proxy copy from the activemq tests module. See NettySimpleAmqpServer(TransportOptions, boolean, boolean, boolean). I noticed that the TestProxy assigns to its listenThread variable in a loop, meaning it overwrites as it goes and will only ever try joining one at close, plus these then spawn other threads it also cant join. Stepping back, I think it would make sense to simplify the proxy to only supporting a single connection, which would reduce its complexity and allow using one thread for more of the work, or perhaps even all of it (could use a read timeout via socket.setSoTimeout to allow servicing both sockets from the middle) which is what we do in the TestAmqpPeer. You would then just use 2 TestProxy instances for the failover test and verify both handle a connection, having the supplier return a handler for each proxy when asked (and throw if it is asked more times than expected), also allowing verifying how often the supplier is called as noted previously. Its good that you found a workaround for the WS TLS issue as I actually think that is one of the more important cases, one of the reasons I pushed for such testing, and so I don't think we would have wanted to add it with that broken if there was possibility it could be resolved soon and included. At the very least we would probably have wanted to instead specifically reject such a config. The most recent activity around the netty issue (on PR 9564) currently suggests a resolution that wouldn't implicitly make it just work in future, but instead enable slightly different usage that could be done to then let it to work. Please just squash the 2nd commit since its just one overall change, plus it will be more helpful later to see that the code was changed while/due-to adding the proxy functionality.
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
