You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by clebertsuconic <gi...@git.apache.org> on 2018/08/20 20:42:19 UTC
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/2254
compatibility fixes and replication fixes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis replication-test
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2254.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2254
----
commit 822589b49d1c92090d3c0e4846fdac9ca3f28ecf
Author: Clebert Suconic <cl...@...>
Date: 2018-08-17T20:45:07Z
replication testing
commit b9f388ac4ac0a2a60b77d35cbb50c503924f7561
Author: Clebert Suconic <cl...@...>
Date: 2018-08-18T04:07:45Z
more fixes
commit 2f46963b74b96d7888a542b893d57ea1149a5650
Author: Clebert Suconic <cl...@...>
Date: 2018-08-18T20:33:08Z
removing un-requiring flush
commit 6a63e6b347d68ce6ca9c3a162c3e4d918864216e
Author: Clebert Suconic <cl...@...>
Date: 2018-08-19T22:16:33Z
fix on journal
commit e4de4a1ea04ec97b5bc86f90494527118cbe83a3
Author: Clebert Suconic <cl...@...>
Date: 2018-08-19T22:35:22Z
fix on JDBC
commit f637d42de866db267afc678c2351be713bc3bfef
Author: Clebert Suconic <cl...@...>
Date: 2018-08-20T02:19:27Z
CheckStyle
commit 9c51a831af3f92a8a11817da68309230331db924
Author: Clebert Suconic <cl...@...>
Date: 2018-08-20T19:53:51Z
System properties support
----
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211419574
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -371,7 +369,7 @@ public Destination getJMSReplyTo() throws JMSException {
String name = address.toString();
// swap the old prefixes for the new ones so the proper destination type gets created
- if (enable1xPrefixes) {
+ if (session.isEnable1xPrefixes()) {
--- End diff --
It's the same.. instead of a boolean per message, I'm using a single property on the session now.
We can improve it later to make sure it's static, but I can't do it ATM becuase of RA and caches of connections.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211420562
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -371,7 +369,7 @@ public Destination getJMSReplyTo() throws JMSException {
String name = address.toString();
// swap the old prefixes for the new ones so the proper destination type gets created
- if (enable1xPrefixes) {
+ if (session.isEnable1xPrefixes()) {
--- End diff --
The more invasive all these changes just for support of 1.x (isnt that the point of the major version change to signal breaking changes)
Would it maybe better to have a 1x jms client module, that has all these bits in so users on 2.0.x dont get affected and also then keeps it all a bit more clean room
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211426369
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -371,7 +369,7 @@ public Destination getJMSReplyTo() throws JMSException {
String name = address.toString();
// swap the old prefixes for the new ones so the proper destination type gets created
- if (enable1xPrefixes) {
+ if (session.isEnable1xPrefixes()) {
--- End diff --
We can look into making an improvement for this. but I don't want to endup with two versions to maintain.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211409233
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
Also this will act as a broken window and encourage further set this way, further causing such issue.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211573418
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Would need a bigger refactoring.
With the extra code the regular case would be kept untouched.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211427175
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
@michaelandrepearce you made a comment I can't find on the web interface here. could only see on email.
```
Eg the jms connection or jms session
```
The Resource Adapter will not have reference to the JMS Session in the same way. CoreSession is the only way to do it.
The other possibility would be to use a metadata, but I did not want to trust a hashmap operation everytime this was called.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211618431
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
I checked your branch, and the method boolean isEnable1xPrefixes(), doesnt seem to be used anymore at all in your new version so could this just be removed now?
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211418260
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
+ private static Logger logger = Logger.getLogger(ArtemisSystemProperties.class);
+
+ static {
+ init();
+ }
+
+ private static ArtemisSystemProperties theInstance;
+
+ private Properties properties;
+ private boolean enable1XPrefixes = false;
--- End diff --
I tried marking the bits that i think are hot paths. Using phone atm so not easiest
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211418220
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
I don't see a difference from the previous code.
This is getJMSDestination. Not really on the hot path.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211417758
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -371,7 +369,7 @@ public Destination getJMSReplyTo() throws JMSException {
String name = address.toString();
// swap the old prefixes for the new ones so the proper destination type gets created
- if (enable1xPrefixes) {
+ if (session.isEnable1xPrefixes()) {
--- End diff --
Hot path here
---
[GitHub] activemq-artemis issue #2254: compatibility fixes and replication fixes
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2254
@michaelandrepearce @mtaylor Let me play with some changes tonight...
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211418620
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -219,7 +221,12 @@ public static SimpleString createTopicAddressFromName(final String name) {
}
public static ActiveMQQueue createQueue(final String address) {
- return new ActiveMQQueue(address);
+
--- End diff --
I did not mean to change this class. accident!
fixing it!
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211418039
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -219,7 +221,12 @@ public static SimpleString createTopicAddressFromName(final String name) {
}
public static ActiveMQQueue createQueue(final String address) {
- return new ActiveMQQueue(address);
+
--- End diff --
Hot path here (esp for spring users)
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211415126
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Does this need to be changable? Could this not just be set in constructor and field made final. As later the check is on hot path.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211422507
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
I got that this is better but tbh the commit that added thay change was merged fairly rapidly too. And once we relesse we get stuck with it. So rather we tidy up as much as possible now.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211411859
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
>>> Also this will act as a broken window and encourage further set this way, further causing such issue.
The system property should change defaults only. Only exception will be the AtiveMQJMSClient.
I don't see any difference than getting a System.property on the class itself instead of encapsulating usage here.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211410036
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
BTW: Two CFs can still have different settings.. on the CF this is just altering the default. You can still switch independently on the CF. The default now comes from a system property instead of a constant variable.
As of on the ActiveMQJMSClient, there's no way to alter the semantic without a system property. But users shouldn't be encouraged to use that class anyways. I'm considering to set it as deprecated.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211421516
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
notice the previous version had a boolean per message. I'm improving this by using nly one at the session.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211692631
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
@michaelandrepearce I will remove it.. all usage through extensions
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211419155
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQDestination.java ---
@@ -219,7 +221,12 @@ public static SimpleString createTopicAddressFromName(final String name) {
}
public static ActiveMQQueue createQueue(final String address) {
- return new ActiveMQQueue(address);
+
--- End diff --
That one is fixed
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211413331
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
+ private static Logger logger = Logger.getLogger(ArtemisSystemProperties.class);
+
+ static {
+ init();
+ }
+
+ private static ArtemisSystemProperties theInstance;
+
+ private Properties properties;
+ private boolean enable1XPrefixes = false;
--- End diff --
this be marked static final and the loading done all in static method.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211419508
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
It can be heavily called in apps doing dynamic re routing.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211423843
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Eg the jms connection or jms session
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211415938
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
+ private static Logger logger = Logger.getLogger(ArtemisSystemProperties.class);
+
+ static {
+ init();
+ }
+
+ private static ArtemisSystemProperties theInstance;
+
+ private Properties properties;
+ private boolean enable1XPrefixes = false;
--- End diff --
Actually.. this is not checked on the hot path.
the one on the ClientSessionImpl is.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211422975
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Also all this 1x stuff is for JMS users right? Could this not just sit in the JMS wrappers only then; and avoid it hitting core client code?
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211494915
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Could the jms session not be the thing referenced in the message, instead of the core session. And then this would allow the custom jms bit to live there, and youd still have access to the core session (though probably better to delegate through)
---
[GitHub] activemq-artemis issue #2254: compatibility fixes and replication fixes
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2254
@michaelandrepearce I have a commit unfinished here. I'm going to sleep and I coudn't finish as I expected.. but hope you get the idea.
I will still need it to be part of the same jar, as the ConnectionFactory would then create a CompatibleConnection or a regular connection, etc.. etc...
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211409599
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
This class is about defaults... The default coming from a system property.
There's no way around a System property. users coded for older servers will need to be switched without changes.
The idea of this class is to use a file on the classLoader.
This is mainly for wildfly. In Wildfly they have set the entire VM to use older properties. ActiveMQJMSClient should be deprecated... as of now I don't see a way we could fix this without System properties.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211417924
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
Hot path here
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211418658
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
Ive marked where hot paths concern me. On phone so may have missed one. Wont get to computer to properly go ovee till tomorrow
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211421243
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
Session is not changeable.
the property on session currently is changeable because of the ResourceAdapter.
We can verify if it can be final there even for RA.
---
[GitHub] activemq-artemis issue #2254: compatibility fixes only
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2254
@michaelandrepearce I will refactor the whole thing... I sent a new PR for what I need now.. and i will redesign this later.
I will probably go with 1.x extensions.. but I'm still thinking about it.
Change anything on the JMS layer requires at least a run of 24 hours on TCK (that's how long it takes).
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211423306
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
This is for JMS users only could all the 1x changes just sit in the jms module, rather thsn spreading out
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211412518
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
My concern is the broken window here. That further people may start abusing this in future developments. Could this be closer coupled with the defaults class or something so its very clear this is only used to override the default
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211416106
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
@michaelandrepearce Notice I'm improving the previous solution that had one boolean on each message.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211423204
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
ClientSession is the only shared object on Every ActiveMQMessage
The other possibility is to revert to the previous change, where you had a boolean on every ActiveMQMessage.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211419727
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
Why does session it need to be changable?
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211416478
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
@michaelandrepearce also, notice this is only affecting the default on ActiveMQConnectionFactory...
There the value is also settable.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211413708
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
+ private static Logger logger = Logger.getLogger(ArtemisSystemProperties.class);
+
+ static {
+ init();
+ }
+
+ private static ArtemisSystemProperties theInstance;
+
+ private Properties properties;
+ private boolean enable1XPrefixes = false;
--- End diff --
@michaelandrepearce The class is singleton.
the reason I did not make it final was because of testcases where I can call init() to reload properties.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211435170
--- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQMessage.java ---
@@ -423,7 +421,7 @@ public Destination getJMSDestination() throws JMSException {
SimpleString address = message.getAddressSimpleString();
SimpleString name = address;
- if (address != null & enable1xPrefixes) {
+ if (address != null & session.isEnable1xPrefixes()) {
--- End diff --
Let me plan with some changes tonight... leave this PR open for now.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211415627
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
+ private static Logger logger = Logger.getLogger(ArtemisSystemProperties.class);
+
+ static {
+ init();
+ }
+
+ private static ArtemisSystemProperties theInstance;
+
+ private Properties properties;
+ private boolean enable1XPrefixes = false;
--- End diff --
Though this is being got on hot path later in message buffers, which is not so good. Having it static final will minimise perf impact
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211617012
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
How about using the ConnectionFactoryOptions to hold this, as this is a product of JMS only?
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211415997
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionImpl.java ---
@@ -241,6 +243,16 @@
// ClientSession implementation
// -----------------------------------------------------------------
+ @Override
+ public boolean isEnable1xPrefixes() {
+ return enable1XPrefixes;
+ }
+
+ @Override
+ public void setEnable1xPrefixes(boolean value) {
--- End diff --
@michaelandrepearce I needed it to be set because of the ResourceAdapter. MDBs.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211409036
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
This class is a bad idea. Having config setting to statics means if two cfs and want different setting for each eg connection to different clusters, then you cant.
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes and replication fix...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2254#discussion_r211413409
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ArtemisSystemProperties.java ---
@@ -0,0 +1,76 @@
+/*
+ * 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.utils;
+
+import java.net.URL;
+import java.util.Properties;
+
+import org.jboss.logging.Logger;
+
+public final class ArtemisSystemProperties {
+
--- End diff --
The reason I encapsulated this.. was because of the file load on the classPath. Wildfly (or anyone else) will be able to specify a properties file on the classLoader and alter this as a System property.
I am also reusing that between the default on CF and the ActiveMQJMSClient (that needs a system property regardless).
---
[GitHub] activemq-artemis issue #2254: compatibility fixes only
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2254
@clebertsuconic you still working on this?
---
[GitHub] activemq-artemis pull request #2254: compatibility fixes only
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2254
---