Conversation
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java
Outdated
Show resolved
Hide resolved
|
can you squash these commits and push -f? |
Yes of course! I am currently taking care of it ;) |
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java
Outdated
Show resolved
Hide resolved
|
@rtista, are you still working on this or should I updated it and send a PR of my own? |
|
This looks good to me. |
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java
Outdated
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java
Outdated
Show resolved
Hide resolved
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java
Outdated
Show resolved
Hide resolved
|
Sorry, I inadvertently closed this :/ |
|
Ok guys, I just rebased against master and I think everything is ready! Whenever you're ready just accept it ;) |
|
@rtista I will run the whole test suite and if it's passing it's a good sign.. takes 2 hours to run it. |
|
@rtista I actually had 3 failures, although I don't know if the BasicSecurityManagerFailoverTest is an intermittent / non related failure yet. but the AmqpFullyQualifiedNameTest is definitely a real failure.
|
|
Thanks for the quick response @clebertsuconic, I was attempting to execute that same test in Intellij but I'm having what I believe is a different failure from yours:
I am probably missing some setup in order to run the test through Intellij. I can see however that the test is attempting to create an invalid FQQN (since an empty address is being passed) and expecting it to fail as in the test doc says "Broker should return exception if no address is passed in FQQN". |
|
I see that happening in Intelij sometimes. I don't know what magic I do. But I mix refreshing the workspace with the proper JDK. Pay attention to the profiles you have on. If you are on jdk8 only select jdk8 as the profile. |
|
I just got it working by re-running install, maybe I missed something. Anyway, I already have that test failing with the expected exception not being thrown. I'll be chasing it tomorrow after work! Thanks for the help! ;) |
|
Hey guys, I believe I've found out the reason to why the test is failing but I'm going to need some help on deciding how to proceed from here. So, according to the Javadoc, the I've tracked the variations in the data flow to two places in the
I am currently unsure as to how to proceed since I do not know if the behavior created by my modifications is expected in the AMQP protocol or not and would like to get some advice from you guys 😄 |
|
The test is trying to open a queue with ::queue.. after your change this is being considered a regular queue, which is not what it should be doing. it seems the opposite of what you're changing. |
|
To do what you're proposing, you would need some pedantic mode where you would throw an exception with something invalidException. however this type of semantic change may break other things.. Do you really want this PR? I think we should consider leaving it as is. |
|
@clebertsuconic I fully understand your concerns, I could also just check for presence of characters on the right side of the separator. This would strengthen the validation by always checking for the queue part at least but ignoring the address which would still allow that test to succesfuly pass. I guess I'd like to hear more opinions though, such as @jbertram's and @michaelandrepearce's, before closing the PR as something that would cause more harm than good 😜 Regarding the PR, I do not "want" the PR, I was just looking to contribute. I picked up this Jira issue as it seemed like a good first issue in a large project such as this one. If you can direct me to other good Jira's for starting up artemis contributions I'd be delighted x) |
|
@rtista I see.. I thought you were having an issue you were facing and wanted this. I will wait on others opinion on this. |
|
In principal, I liked the idea of tightening the rules for FQQN validation, but it's obviously causing other issues. Perhaps the method should throw an exception if the input is not a valid FQQN but it still contains the separator (i.e. |
|
Hey @jbertram and @clebertsuconic! I partially agree with the desired behavior you guys described and I don't mind implementing it and dealing with the errors that may arise from it. However, most usages of the isfullyQualified method are in if statements and it seems to me that adding the exception throwing behavior will render this code (which calls isfullyQualified) confusing. I believe it would make more sense to throw an exception in methods such as toFullyQualified, extractQueueName and extractAddressName than in isFullyQualified since the latter is used by its callers to create logic variations when a provided string is not in FQQN format. While some clients may wish to throw an exception because they only support FQQN syntax, others may wish to fallback to another behavior such as creating an ANYCAST queue with the string provided. I have tracked usages of CompositeAddress.isfullyQualified to the following files: [artista@astartes activemq-artemis]$ find . -type f -name "*.java" -exec grep -l "CompositeAddress.isFullyQualified(" {} \;
./artemis-cli/src/test/java/org/apache/activemq/cli/test/CliProducerTest.java
./artemis-cli/src/test/java/org/apache/activemq/cli/test/MessageSerializerTest.java
./artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java
./artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQSession.java
./artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
./artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
./artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java
./artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java
./artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompSession.java
./artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java
./artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
./artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java
./artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.javaThe usages themselves are as follow where it is possible to see the high amount of if statements: [artista@astartes activemq-artemis]$ grep isFullyQualified -r | grep ".java:"
artemis-cli/src/test/java/org/apache/activemq/cli/test/CliProducerTest.java: List<Message> received = consumeMessages(session, address, TEST_MESSAGE_COUNT, CompositeAddress.isFullyQualified(address));
artemis-cli/src/test/java/org/apache/activemq/cli/test/MessageSerializerTest.java: List<Message> received = consumeMessages(session, address, TEST_MESSAGE_COUNT, CompositeAddress.isFullyQualified(address));
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java: if (CompositeAddress.isFullyQualified(address)) {
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java: if (CompositeAddress.isFullyQualified(name)) {
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java: public static boolean isFullyQualified(String address) throws ActiveMQInvalidQueueConfiguration {
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java: public static boolean isFullyQualified(SimpleString address) throws ActiveMQInvalidQueueConfiguration {
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/CompositeAddress.java: return address != null && isFullyQualified(address.toString());
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQSession.java: } else if (CompositeAddress.isFullyQualified(address)) { // it could be a topic using FQQN
artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQSession.java: if (!CompositeAddress.isFullyQualified(dest.getAddress())) {
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java: if (CompositeAddress.isFullyQualified(source.getAddress())) {
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java: if (CompositeAddress.isFullyQualified(physicalName)) {
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQSession.java: if (CompositeAddress.isFullyQualified(queueName.toString())) {
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java: if ((CompositeAddress.isFullyQualified(destination) || effectiveAddressRoutingType == RoutingType.ANYCAST) && addressSettings.isAutoCreateQueues() && manager.getServer().locateQueue(simpleDestination) == null) {
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompSession.java: if (multicast && !CompositeAddress.isFullyQualified(destination)) {
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/BindingsImpl.java: } else if (CompositeAddress.isFullyQualified(message.getAddress())) {
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java: if (!newFQQN && CompositeAddress.isFullyQualified(address.toString())) {
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java: securityCheck(CompositeAddress.extractAddressName(art.getName()), CompositeAddress.isFullyQualified(art.getName()) ? CompositeAddress.extractQueueName(art.getName()) : null, CheckType.SEND, this);
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: public void testIsFullyQualifiedInvalidFQQN() throws ActiveMQInvalidQueueConfiguration {
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: CompositeAddress.isFullyQualified(failureScenario);
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: public void testIsFullyQualifiedNullQueue() throws ActiveMQInvalidQueueConfiguration {
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: CompositeAddress.isFullyQualified(nullableQueueScenario);
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: public void testIsFullyQualifiedNullAddress() throws ActiveMQInvalidQueueConfiguration {
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: CompositeAddress.isFullyQualified(nullableAddressScenario);
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: public void testIsFullyQualifiedSuccess() throws ActiveMQInvalidQueueConfiguration {
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/CompositeAddressTest.java: assertTrue("Valid FQQN was not validated.", CompositeAddress.isFullyQualified(successScenario));Let me know what you think about this. |
|
-1 to throwing exceptions and changing behaviour. It will cause the jvm to capture stack traces unneeded. Remember this is hot path code that affects end2end message latency/perf Id like to see some end2end perf stats on changes here too before it can be merged. Not just for fqqn but non fqqn as well. |
|
Hey @michaelandrepearce, after I developed the code I ran some arguably worthy performance tests to the function. They are not e2e however. I simply recorded the execution time of the function for one hundred million executions and calculated the mean. Here's the code I used: int sum = 0;
final int TEST_TIMES = 100000000;
final String TEST_STRING = "address::queue";
long startTime;
long stopTime;
for (int i = 0 ; i < TEST_TIMES ; i++) {
startTime = System.nanoTime();
CompositeAddress.isFullyQualifiedOld(TEST_STRING);
stopTime = System.nanoTime();
sum += (stopTime - startTime);
}
System.out.println("Before FQQN Performance: " + (sum / TEST_TIMES));
sum = 0;
for (int i = 0 ; i < TEST_TIMES ; i++) {
startTime = System.nanoTime();
CompositeAddress.isFullyQualified(TEST_STRING);
stopTime = System.nanoTime();
sum += (stopTime - startTime);
}
System.out.println("After FQQN Performance: " + (sum / TEST_TIMES));I executed this code for "address::queue", "::queue", "address::" and null and here are the results:
I understand however that these tests might not carry any real meaning to you, but if you're able to direct me to an example of what you need I can surely try to provide those results to you ;) |
|
Hi @rtista please use JMH to produce a valid benchmark: see |
|
@franz1981 Can we have some e2e ones. These microbenchmarks dont cater for the system as a whole. Of recent weve dropped about 12k in throughput in the last yrs (see the public softwaremill perf report this yr (52k) vs 2 yrs back (64k) ) so we gotta get better at ensuring we dont regress or slowley erode e2e perf. See here https://softwaremill.com/mqperf/ vs |
|
@michaelandrepearce i have had some personal issue this year so haven't yet fixed that drop, will look at it with the new year and I am trusty can be fixed. At least we don't exhibit the perf scalability issue shown on https://issues.apache.org/jira/browse/ARTEMIS-2852. |
|
@michaelandrepearce And I was wrong: I see that Artemis 2.2.0 is using G1GC: The only relevant difference is the string deduplication (that can add some latency spike) and the heap size, global max size. |
|
Hey guys, hope you all had a Merry Christmas! ;) @franz1981 I think I was able to implement the JMH tests you were looking for, here's the JMH benchmark class I created: /*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.activemq.artemis.tests.performance.jmh;
import java.util.concurrent.TimeUnit;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
public class CompositeAddressBenchmark {
/* The FQQN separator */
public static final String SEPARATOR = "::";
/* The FQQN separator length */
public static final int SEPARATOR_LENGTH = SEPARATOR.length();
@State(Scope.Benchmark)
public static class BenchmarkState {
@Param({"address::queue", "address::", "::queue", "::"})
public String queue;
}
/**
* Code for isFullyQualified before Artemis-1883 changes.
* @return boolean
*/
@Benchmark
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(value = 1, warmups = 1)
@Warmup(iterations = 1)
public static boolean isFullyQualifiedBefore(BenchmarkState state) {
return state.queue != null && state.queue.contains(SEPARATOR);
}
/**
* Code for isFullyQualified after Artemis-1883 changes.
* @return boolean
*/
@Benchmark
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(value = 1, warmups = 1)
@Warmup(iterations = 1)
public static boolean isFullyQualifiedAfter(BenchmarkState state) {
if (state.queue == null) {
return false;
}
// If address length is incapable of holding the separator and two more characters
if (state.queue.length() < SEPARATOR_LENGTH + 2) {
return false;
}
// If separator is not found or is at the start or end of string then this is not a FQQN
int index = state.queue.indexOf(SEPARATOR, 1);
return index != -1 && index != state.queue.length() - SEPARATOR_LENGTH;
}
}And here are the results: I haven't pushed the class code as I thought you'd probably want to mess with the warmup, fork and iterations parameters I guess in order to improve the results. Also, @franz1981 is there any way you can direct me to a framework for the E2E tests @michaelandrepearce is looking for? |
|
You've done some good work on this PR, @rtista. However, I think that this is ultimately a solution looking for a problem that doesn't actually exist. As currently written and used throughout the code-base, the Given this I'm closing this PR. |
|
To be clear, I committed a38f009 to clarify the docs. |
@jbertram Not a problem! Glad of what I've already learned from you guys so far! I'll be picking other Jira Issues for fun 😄 |
6 participants
This pull request improves the validation of Fully Qualified Queue Name (FQQN) strings as per issue ARTEMIS-1883 by validating the presence of content in both sides of the "::" separator.