[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[asfgit]

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...

[mtaylor]

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...

[mtaylor]

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...

[michaelandrepearce]

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 pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[michaelandrepearce]

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? 


---

[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[clebertsuconic]

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 pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[clebertsuconic]

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);
 

[GitHub] activemq-artemis pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[mtaylor]

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 pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[jmesnil]

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...

[mtaylor]

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...

[mtaylor]

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 pull request #1783: ARTEMIS-1611 Added support for 1.x tran...

[jmesnil]

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...

[mtaylor]

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...

[jmesnil]

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...

[mtaylor]

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 
Date:   2018-01-15T16:36:01Z

ARTEMIS-1611 Added support for 1.x transformer API




---