Hi Arthur, with a week delay, i now created the pull request: https://issues.apache.org/jira/browse/AMQ-6939
please have a look. regards, Max ----- In Reply to Original Message ----- <cae+atikg7fi+psbti_8j8jzavlpp4pdydwkx9mtbc8zqybo...@mail.gmail.com> From: Arthur Naseef <[email protected]> To: dev <[email protected]> <[email protected]>; Sent: Fri, 16 Mar 2018 16:44:05 -0700 Subject: Re: question regarding TcpTransportFactory.java > If you want to create a PR for just the log message, I would merge it. > > Terse code comments that help clarify what it's doing would be welcome as > well. Perhaps a comment that clarifies the format is > <schema>://<host>:<post>/<bind-host-address>:<bind-host-port>... > > Art > > > On Fri, Mar 16, 2018 at 3:47 PM, Max-Julian Pogner <[email protected]> > wrote: > >> >> Starting at the end and working back. This code is very old (and stable). >> I wouldn't touch it unless we proved there was a problem with it. >> >> >> Apart from the code-comments, there are two changes in the diff: >> >> 1) treat a path "/" like the empty path. >> >> this part is an improvement i would think makes the code better, but >> does not solve any problem. As your argument is very valid, this part >> could be dropped. >> >> >> 2) change of logged warning formatting >> >> as it stands now in the master branch, the LOG.warn() line always >> discards e.getMessage(). According to the specification of >> Logger.warn() the additional parameters are only used according to >> placeholders in the format-string. Since there is no placeholder in the >> format-string, the string from e.getMessage() is discarded. >> >> this is definitely a problem, albeit with no consequences to a properly >> configured system. However, this missing error message reason >> is the cause of this mail thread. >> I would very much like to avoid other people wondering about the >> same problem. >> >> >> 2a) the code comments are added sugar, which of-course do not >> solve any immediate problems. >> I am unsure how many people might also look at the code in the >> future and would be happy about some additional comments. >> >> >> As a Side-Note: >> https://issues.apache.org/jira/browse/AMQ-2256 >> It seems that before, the exception was always logged, and with >> https://issues.apache.org/jira/browse/AMQ-2256?focusedCommentId=12999853&; >> page=com.atlassian.jira.plugin.system.issuetabpanels: >> comment-tabpanel#comment-12999853 >> the missing format-placeholder was introduced. >> >> >> regards, >> >> Max >> >> >> >> As for the "auto" transport - I've never used it. I find that the simple >> 'failover' + 'tcp' works great. I suspect, based on the name, the auto >> transport is auto-detecting. But why do I have a client connecting to >> something for which it doesn't know ahead of time which protocol to use? >> Fewer variables = easier to maintain and easier to diagnose problems ;-). >> >> Other uses of the path portion of the url - not a concern; as you guessed, >> the path-handling is specific to the transport (identified by scheme) >> itself. If something else is forming TCP url's that don't conform to the >> TCP transport's implementation... well - that's just a bad idea. >> >> Art >> >> >> On Fri, Mar 16, 2018 at 3:07 PM, Max-Julian Pogner <[email protected]> >> <[email protected]> >> wrote: >> >> >> Ah, now i see! >> >> >> But i wonder: is there a conflict when using the URI like this, because >> there might be already another use of the path-part w.r.t to the active-mq >> configuration. However, it could be argued that this use is restricted to >> the "tcp" scheme and as long as there is no conflicting using _within_ the >> "tcp" scheme it'll work out. >> >> However, what if the "auto" schema was specified in the original config >> file, and maybe the "auto" scheme passes the uri-path along to the detected >> actual scheme, and then two possible actual schemes have surprisingly >> different interpretations of the uri-path. >> >> >> *) the URI location passed as parameter to the createTransport function: >> is the location.getScheme() always "tcp"? even if the original >> configuration is something like "auto://localhost:61616"? >> >> *) if the path part of location indeed has no conflicting purpose, i would >> propose - as a first idea - a patch similar to the diff attached. (Note: i >> didn't compile yet, let alone test) >> >> >> with regards, >> >> Max >> >> >> >> [ w.r.t TcpTransport ] >> >> >> OK, try a url like this: >> >> tcp://localhost:61616/localhost:56565 >> >> >> You'll find it connects to localhost:61616 and binds to localhost:56565 >> >> I don't see any documentation on the website for this feature though. >> >> Art >> >> >> >> >> >> >>
