activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max-Julian Pogner <max-jul...@pogner.at>
Subject Re: question regarding TcpTransportFactory.java
Date Fri, 16 Mar 2018 22:47:16 GMT
 
> 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 <max-julian@pogner.at>
> 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
>>
>>
>>
>>


Mime
View raw message