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 > > > > > > >
