ARTEMIS-1422 Fix match change to support wildcard config #1537
Conversation
michaelandrepearce
force-pushed
the
Match
branch
6 times, most recently
from
85f75d4
to
4ff9553
Compare
| @@ -94,6 +99,11 @@ | |||
| */ | |||
| private final ArrayList<HierarchicalRepositoryChangeListener> listeners = new ArrayList<>(); | |||
|
|
|||
| public HierarchicalObjectRepository(final WildcardConfiguration wildcardConfiguration) { | |||
There was a problem hiding this comment.
I'd rather see a default constructor which calls this one so all the places in the code-base which use the default WildcardConfiguration don't have to be changed to pass in new WildcardConfiguration() or null.
There was a problem hiding this comment.
Fair enough I was divided at the time between the two approaches, will change
There was a problem hiding this comment.
have updated PR with this.
|
|
||
|
|
||
| <wildcard-addresses> | ||
| <enabled>true</enabled> |
There was a problem hiding this comment.
FWIW, the matching logic will use the configured <delimiter>, <any-words>, and <single-word> regardless of how <enabled> is set. However, the routing logic won't. Not sure if we want to make those semantics match (no pun intended).
There was a problem hiding this comment.
What do you do if false? Don't match? As that's what filtering does, it disables the ability to filter, it doesn't set the values to a default.
There was a problem hiding this comment.
The original flag was better name routing-enabled, maybe the config option should be renamed? (With still supporting enabled, as not major release, so can't break bits)
There was a problem hiding this comment.
I believe the only place isEnabled is checked is when the PostOffice is constructed. If it is enabled then a org.apache.activemq.artemis.core.postoffice.impl.WildcardAddressManager is used otherwise a org.apache.activemq.artemis.core.postoffice.impl.SimpleAddressManager is used. This effectively enables/disables wildcard address routing. This is technically different than address matching.
There was a problem hiding this comment.
I agree...the original parameter name was better. The fact that it was changed is probably also an indication as to why the matching logic was left untouched.
There was a problem hiding this comment.
question is do we want to
- rename the config sub element to "routing-enabled" (similar to the current deprecated higher level, we think is a better name) to make this clearer, but keep it local to the sub config area. (for back compatibility we don't remove enabled - just don't advertise it/deprecate it)
- just we need to make it clear in doc for the current "enabled"
- Un-deprecate the old higher level config which we both believe is better named.
my preference is 1,3,2 - ill look to do 1 unless strong opinion
There was a problem hiding this comment.
I think your first option is the best. My overall order preference would be 1, 2, 3.
There was a problem hiding this comment.
have updated pr with option 1.
michaelandrepearce
force-pushed
the
Match
branch
11 times, most recently
from
ca3a30f
to
fc7605c
Compare
|
@jbertram @mtaylor @clebertsuconic can someone either increase the timeout of PR builds, or remove under powered nodes such as "ubuntu-eu3" from doing builds. Other nodes like H25 seem fine. Having build failure due to timeout for exactly this, which seems other PR's also seeing, is a little painful. |
michaelandrepearce
force-pushed
the
Match
branch
from
fc7605c
to
6d16a3d
Compare
|
I removed ubuntu-eu3 from the list of nodes where the PR build will run. Let me know if there are any other nodes where timeouts happen often. |
michaelandrepearce
force-pushed
the
Match
branch
from
6d16a3d
to
d4cdb89
Compare
|
@jbertram cheers that's helped |
|
@jbertram you ok to merge this, seems no further review comments |
No description provided.