You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by Max-Julian Pogner <ma...@pogner.at> on 2018/03/16 20:20:21 UTC
question regarding TcpTransportFactory.java
Hi there!
while investigating unintended behaviour of the program i am currently
writing on, i stumbled over code in TcpTransportFactory.java (in
activemq-client) where i cannot quite ascertain the author's intention.
*) what is a "local location uri"? (or is it just any URI, but then what
does the word "local location" mean?)
*) why is the port parsed as part of the path, isn't the java.net.URI
object given as parameter already supposed to have the schema, path,
port, etc separately?
I would be glad, if someone could give me a hint or two.
as a quick reference to the relevant code:
<https://git-wip-us.apache.org/repos/asf?p=activemq.git;a=blob;f=activemq-client/src/main/java/org/apache/activemq/transport/tcp/TcpTransportFactory.java;h=ae555fdf1b8527486b37ef6dab95f4860af9f6db;hb=HEAD>
lines 125 to 144
with kind regards,
Max
Re: question regarding TcpTransportFactory.java
Posted by Max-Julian Pogner <ma...@pogner.at>.
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 -----
<CA...@mail.gmail.com>
From: Arthur Naseef <ar...@amlinv.com>
To: dev <de...@activemq.apache.org> <de...@activemq.apache.org>;
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 <ma...@pogner.at>
> 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 <ma...@pogner.at> <ma...@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
>>
>>
>>
>>
>>
>>
>>
Re: question regarding TcpTransportFactory.java
Posted by Arthur Naseef <ar...@amlinv.com>.
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 <ma...@pogner.at>
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 <ma...@pogner.at> <ma...@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
>
>
>
>
>
>
>
Re: question regarding TcpTransportFactory.java
Posted by Max-Julian Pogner <ma...@pogner.at>.
> 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 <ma...@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
>>
>>
>>
>>
Re: question regarding TcpTransportFactory.java
Posted by Arthur Naseef <ar...@amlinv.com>.
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.
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 <ma...@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
>
>
>
>
Re: question regarding TcpTransportFactory.java
Posted by Max-Julian Pogner <ma...@pogner.at>.
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
>
Re: question regarding TcpTransportFactory.java
Posted by Arthur Naseef <ar...@amlinv.com>.
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
On Fri, Mar 16, 2018 at 2:27 PM, Arthur Naseef <ar...@amlinv.com> wrote:
> Looking at TcpTransport, I can see that it accepts both remoteLocation and
> localLocation.
>
> If localLocation is defined, it binds the local end of the socket to that
> address.
>
> As for the logic in the factory, I'm still trying to decipher it fully.
>
> Art
>
>
> On Fri, Mar 16, 2018 at 1:20 PM, Max-Julian Pogner <ma...@pogner.at>
> wrote:
>
>> Hi there!
>>
>> while investigating unintended behaviour of the program i am currently
>> writing on, i stumbled over code in TcpTransportFactory.java (in
>> activemq-client) where i cannot quite ascertain the author's intention.
>>
>> *) what is a "local location uri"? (or is it just any URI, but then what
>> does the word "local location" mean?)
>> *) why is the port parsed as part of the path, isn't the java.net.URI
>> object given as parameter already supposed to have the schema, path,
>> port, etc separately?
>>
>> I would be glad, if someone could give me a hint or two.
>>
>> as a quick reference to the relevant code:
>> <https://git-wip-us.apache.org/repos/asf?p=activemq.git;a=
>> blob;f=activemq-client/src/main/java/org/apache/activemq/tra
>> nsport/tcp/TcpTransportFactory.java;h=ae555fdf1b8527486b37ef
>> 6dab95f4860af9f6db;hb=HEAD>
>> lines 125 to 144
>>
>> with kind regards,
>>
>> Max
>>
>>
>>
>>
>
Re: question regarding TcpTransportFactory.java
Posted by Arthur Naseef <ar...@amlinv.com>.
Looking at TcpTransport, I can see that it accepts both remoteLocation and
localLocation.
If localLocation is defined, it binds the local end of the socket to that
address.
As for the logic in the factory, I'm still trying to decipher it fully.
Art
On Fri, Mar 16, 2018 at 1:20 PM, Max-Julian Pogner <ma...@pogner.at>
wrote:
> Hi there!
>
> while investigating unintended behaviour of the program i am currently
> writing on, i stumbled over code in TcpTransportFactory.java (in
> activemq-client) where i cannot quite ascertain the author's intention.
>
> *) what is a "local location uri"? (or is it just any URI, but then what
> does the word "local location" mean?)
> *) why is the port parsed as part of the path, isn't the java.net.URI
> object given as parameter already supposed to have the schema, path,
> port, etc separately?
>
> I would be glad, if someone could give me a hint or two.
>
> as a quick reference to the relevant code:
> <https://git-wip-us.apache.org/repos/asf?p=activemq.git;
> a=blob;f=activemq-client/src/main/java/org/apache/activemq/transport/tcp/
> TcpTransportFactory.java;h=ae555fdf1b8527486b37ef6dab95f4
> 860af9f6db;hb=HEAD>
> lines 125 to 144
>
> with kind regards,
>
> Max
>
>
>
>