You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by mtaylor <gi...@git.apache.org> on 2018/01/17 10:40:22 UTC
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
GitHub user mtaylor opened a pull request:
https://github.com/apache/activemq-artemis/pull/1783
ARTEMIS-1611 Added support for 1.x transformer API
I've added support for the old API back. There was no real need to remove it other than some internal refactoring. I haven't written a test as I'll need to compile against 1.x classes. I could probably do this using @clebertsuconic new test suite. But it seems a little overkill for this, I've built and tested manually,
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mtaylor/activemq-artemis 1xTransformerAPI
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1783.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 #1783
----
commit a360bc56d8d79764926455e788851f0dd4db0080
Author: Martyn Taylor <mt...@...>
Date: 2018-01-15T16:36:01Z
ARTEMIS-1611 Added support for 1.x transformer API
----
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162028476
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/Transformer.java ---
@@ -19,10 +19,18 @@
import java.util.Map;
import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerMessage;
public interface Transformer {
--- End diff --
The interface that needs to support backwards compatibility is https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/Transformer.java not this one (this interface is new in Artemis 2.x)
I was suggesting to add default methods to the cluster's interface instead of adding deprecated method to the new one.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162024692
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/Transformer.java ---
@@ -19,10 +19,18 @@
import java.util.Map;
import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerMessage;
public interface Transformer {
--- End diff --
I'm not sure what you mean here Jeff. Perhaps you could provide an example? Note that I was trying to keep this change as isolated as possible. The only thing I have changed is the transformer interface and added a couple extra classes back.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r163036074
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/ServerMessageImpl.java ---
@@ -0,0 +1,190 @@
+/*
+ * 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.core.server.transformer;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.message.impl.CoreMessage;
+import org.apache.activemq.artemis.core.message.impl.MessageInternalImpl;
+import org.apache.activemq.artemis.core.paging.PagingStore;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.server.ServerMessage;
+
+@Deprecated
--- End diff --
Can do.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
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/1783#discussion_r163011302
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/TypedProperties.java ---
@@ -23,4 +23,7 @@
@Deprecated
public class TypedProperties extends org.apache.activemq.artemis.utils.collections.TypedProperties {
+ public TypedProperties(final org.apache.activemq.artemis.utils.collections.TypedProperties other) {
--- End diff --
Should add the default empty constructor. By adding this without explicit defining the empty it is being removed.
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
@jmesnil @mtaylor what about adding ServerMessage as a class.. and only add the methods that matter...
all these other methods would fail with a NotImplementedException... perhaps they should fail at compilation time instead?
I would add ServerMessage as a class.. have it deprecated and only add what you need.
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
@mtaylor I assume you still working on this, as the TODO is still there?
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
I meant to merge this. If someone could merge it please ?
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162356443
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/ServerMessageImpl.java ---
@@ -0,0 +1,192 @@
+/*
+ * 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.core.server.transformer;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.message.impl.CoreMessage;
+import org.apache.activemq.artemis.core.message.impl.MessageInternalImpl;
+import org.apache.activemq.artemis.core.paging.PagingStore;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.server.ServerMessage;
+import org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
--- End diff --
Yes this was a mistake. Fixed it.
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
@jmesnil Made the changes you suggested. @clebertsuconic I agree with @jmesnil that adding back the original types is the clearest and safest thing to do, with the disadvantage of add more deprecated code back.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1783
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162051266
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/Transformer.java ---
@@ -19,10 +19,18 @@
import java.util.Map;
import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerMessage;
public interface Transformer {
--- End diff --
@jmesnil Actually, I get what you're saying. Move all the default methods into the old interface.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
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/1783#discussion_r162391229
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/ServerMessageImpl.java ---
@@ -0,0 +1,190 @@
+/*
+ * 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.core.server.transformer;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.message.impl.CoreMessage;
+import org.apache.activemq.artemis.core.message.impl.MessageInternalImpl;
+import org.apache.activemq.artemis.core.paging.PagingStore;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.server.ServerMessage;
+
+@Deprecated
--- End diff --
can you add a strong statement on javadoc? something .. DO NOT USE, for backward compatibility only!
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
Will do
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162015472
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/Transformer.java ---
@@ -19,10 +19,18 @@
import java.util.Map;
import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerMessage;
public interface Transformer {
--- End diff --
Maybe you should change only the old `server.cluster.Transformer` interface and keep this new one pristine?
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162103683
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/ServerMessageImpl.java ---
@@ -0,0 +1,192 @@
+/*
+ * 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.core.server.transformer;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.message.impl.CoreMessage;
+import org.apache.activemq.artemis.core.message.impl.MessageInternalImpl;
+import org.apache.activemq.artemis.core.paging.PagingStore;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.server.ServerMessage;
+import org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
--- End diff --
We should not depend on sun package. Maybe use `java.lang.UnsupportedOperationException` instead?
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r162048935
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/Transformer.java ---
@@ -19,10 +19,18 @@
import java.util.Map;
import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerMessage;
public interface Transformer {
--- End diff --
@jmesnil Perhaps I am missing something here. But, this new interface is what we use internally. If I was to add these changes to the old interface I would need to check the Transformer Type at runtime and do a cast before calling the appropriate method. I wanted to avoid this as it has impact on performance. The way it is at the moment there's only a performance hit when using the 1.x interfaces.
---
[GitHub] activemq-artemis issue #1783: ARTEMIS-1611 Added support for 1.x transformer...
Posted by jmesnil <gi...@git.apache.org>.
Github user jmesnil commented on the issue:
https://github.com/apache/activemq-artemis/pull/1783
@clebertsuconic Martyn's is adding backwards compatibility by putting back the exact types. That's the safest way to ensure backwards compatibility works.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1783#discussion_r163035962
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/TypedProperties.java ---
@@ -23,4 +23,7 @@
@Deprecated
public class TypedProperties extends org.apache.activemq.artemis.utils.collections.TypedProperties {
+ public TypedProperties(final org.apache.activemq.artemis.utils.collections.TypedProperties other) {
--- End diff --
Good shout @michaelandrepearce. I missed that. Will update accordingly.
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
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/1783#discussion_r162389643
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/transformer/ServerMessageImpl.java ---
@@ -0,0 +1,190 @@
+/*
+ * 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.core.server.transformer;
+
+import org.apache.activemq.artemis.api.core.ICoreMessage;
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.message.impl.CoreMessage;
+import org.apache.activemq.artemis.core.message.impl.MessageInternalImpl;
+import org.apache.activemq.artemis.core.paging.PagingStore;
+import org.apache.activemq.artemis.core.server.MessageReference;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.server.ServerMessage;
+
+@Deprecated
+public class ServerMessageImpl extends MessageInternalImpl implements ServerMessage {
+
+ private CoreMessage message;
+
+ private boolean valid = false;
+
+ public boolean isValid() {
+ return false;
+ }
+
+ @Override
+ public ICoreMessage getICoreMessage() {
+ return message;
+ }
+
+ public ServerMessageImpl(Message message) {
+ super(message.toCore());
+ this.message = (CoreMessage) message.toCore();
+ }
+
+ @Override
+ public ServerMessage setMessageID(long id) {
+ message.setMessageID(id);
+ return this;
+ }
+
+ @Override
+ public MessageReference createReference(Queue queue) {
+ throw new UnsupportedOperationException();
+ }
+
+ /**
+ * This will force encoding of the address, and will re-check the buffer
+ * This is to avoid setMessageTransient which set the address without changing the buffer
+ *
+ * @param address
+ */
+ @Override
+ public void forceAddress(SimpleString address) {
+ message.setAddress(address);
+ }
+
+ @Override
+ public int incrementRefCount() throws Exception {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int decrementRefCount() throws Exception {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int incrementDurableRefCount() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int decrementDurableRefCount() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public ServerMessage copy(long newID) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public ServerMessage copy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int getMemoryEstimate() {
+ return message.getMemoryEstimate();
+ }
+
+ @Override
+ public int getRefCount() {
+ return message.getRefCount();
+ }
+
+ @Override
+ public ServerMessage makeCopyForExpiryOrDLA(long newID,
+ MessageReference originalReference,
+ boolean expiry,
+ boolean copyOriginalHeaders) throws Exception {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void setOriginalHeaders(ServerMessage otherServerMessage, MessageReference originalReference, boolean expiry) {
+
+ ICoreMessage other = otherServerMessage.getICoreMessage();
+
+ SimpleString originalQueue = other.getSimpleStringProperty(Message.HDR_ORIGINAL_QUEUE);
+
+ if (originalQueue != null) {
+ message.putStringProperty(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+ } else if (originalReference != null) {
+ message.putStringProperty(Message.HDR_ORIGINAL_QUEUE, originalReference.getQueue().getName());
+ }
+
+ if (other.containsProperty(Message.HDR_ORIG_MESSAGE_ID)) {
+ message.putStringProperty(Message.HDR_ORIGINAL_ADDRESS, other.getSimpleStringProperty(Message.HDR_ORIGINAL_ADDRESS));
+
+ message.putLongProperty(Message.HDR_ORIG_MESSAGE_ID, other.getLongProperty(Message.HDR_ORIG_MESSAGE_ID));
+ } else {
+ message.putStringProperty(Message.HDR_ORIGINAL_ADDRESS, new SimpleString(other.getAddress()));
+
+ message.putLongProperty(Message.HDR_ORIG_MESSAGE_ID, other.getMessageID());
+ }
+
+ // reset expiry
+ message.setExpiration(0);
+
+ if (expiry) {
+ long actualExpiryTime = System.currentTimeMillis();
+
+ message.putLongProperty(Message.HDR_ACTUAL_EXPIRY_TIME, actualExpiryTime);
+ }
+
+ // TODO ASk clebert
+ //message.bufferValid = false;
--- End diff --
@mtaylor remove it... message.putStringProperty, setExpiration... or any other change will set bufferValid = false on the delegation.. so you can safely remove this.. and remove this TODO
---
[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...
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/1783#discussion_r163010905
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/TypedProperties.java ---
@@ -23,4 +23,7 @@
@Deprecated
public class TypedProperties extends org.apache.activemq.artemis.utils.collections.TypedProperties {
+ public TypedProperties(final org.apache.activemq.artemis.utils.collections.TypedProperties other) {
--- End diff --
What about keeping the default empty constructor?
---