You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2018/03/21 08:23:51 UTC
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/1960
ARTEMIS-406 STOMP acknowledgements should support transactions
- Add support for transactional STOMP acks/nacks
- Fixed some stomp test issues
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis a_a416
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1960.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 #1960
----
----
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1960#discussion_r176096108
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/CommandType.java ---
@@ -0,0 +1,84 @@
+/*
+ * 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.protocol.stomp;
+
+public enum CommandType {
+
+ CONNECT(Stomp.Commands.CONNECT),
+ SEND(Stomp.Commands.SEND),
+ DISCONNECT(Stomp.Commands.DISCONNECT),
+ SUBSCRIBE(Stomp.Commands.SUBSCRIBE),
+ UNSUBSCRIBE(Stomp.Commands.UNSUBSCRIBE),
+ BEGIN(Stomp.Commands.BEGIN),
+ COMMIT(Stomp.Commands.COMMIT),
+ ABORT(Stomp.Commands.ABORT),
+ ACK(Stomp.Commands.ACK),
+ NACK(Stomp.Commands.NACK),
+ STOMP(Stomp.Commands.STOMP),
+ CONNECTED(Stomp.Responses.CONNECTED),
+ ERROR(Stomp.Responses.ERROR),
+ MESSAGE(Stomp.Responses.MESSAGE),
+ RECEIPT(Stomp.Responses.RECEIPT);
+
+ private String type;
+
+ CommandType(String command) {
+ type = command;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+
+ public static CommandType getType(String command) {
+ switch (command) {
--- End diff --
ok fair point. I'll remove this.
---
[GitHub] activemq-artemis issue #1960: ARTEMIS-406 STOMP acknowledgements should supp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1960
@clebertsuconic sorry I forgot it.
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1960#discussion_r176095964
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompSession.java ---
@@ -154,12 +154,16 @@ public int sendMessage(MessageReference ref,
encoder.encode(buffer, bodySize);
encoder.close();
} else {
- buffer = coreMessage.getReadOnlyBodyBuffer();
+ if (Boolean.TRUE.equals(serverMessage.getBooleanProperty(Message.HDR_LARGE_COMPRESSED))) {
+ buffer = coreMessage.getBodyBuffer();
--- End diff --
this is because we need to decompress the content before send to clients. If readonly we can't write into it.
---
[GitHub] activemq-artemis issue #1960: ARTEMIS-406 STOMP acknowledgements should supp...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1960
@gaohoward you mentioned you were closing this, but still open?
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1960#discussion_r176097804
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompTransaction.java ---
@@ -0,0 +1,82 @@
+/*
+ * 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.protocol.stomp;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerSession;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Control's stomp transaction processing
+ * it goes with stomp connections.
+ * it stores acks, sends, nacks
+ * during commit it applies those to core sessions.
+ * because each stomp connection uses only one session
+ * we can't rely on core session to manage multiple
+ * tx's.
+ */
+public class StompTransaction {
+
+ private List<StompAck> acks = new ArrayList<>();
--- End diff --
The StompTransaction is just a data holding acks/sends, the real transaction is delegated to core session. Using XA would be unnecessary as we are not dealing with global transactions.
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
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/1960#discussion_r176080680
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompTransaction.java ---
@@ -0,0 +1,82 @@
+/*
+ * 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.protocol.stomp;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerSession;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Control's stomp transaction processing
+ * it goes with stomp connections.
+ * it stores acks, sends, nacks
+ * during commit it applies those to core sessions.
+ * because each stomp connection uses only one session
+ * we can't rely on core session to manage multiple
+ * tx's.
+ */
+public class StompTransaction {
+
+ private List<StompAck> acks = new ArrayList<>();
--- End diff --
Instead of storing these here.. Can't you do like XA is doing? Recover the transaction... and do the proper Transaction object?
Do you really need a StompTransaction? can't you reuse the actual Transaction we already have?
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
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/1960#discussion_r176080114
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/CommandType.java ---
@@ -0,0 +1,84 @@
+/*
+ * 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.protocol.stomp;
+
+public enum CommandType {
+
+ CONNECT(Stomp.Commands.CONNECT),
+ SEND(Stomp.Commands.SEND),
+ DISCONNECT(Stomp.Commands.DISCONNECT),
+ SUBSCRIBE(Stomp.Commands.SUBSCRIBE),
+ UNSUBSCRIBE(Stomp.Commands.UNSUBSCRIBE),
+ BEGIN(Stomp.Commands.BEGIN),
+ COMMIT(Stomp.Commands.COMMIT),
+ ABORT(Stomp.Commands.ABORT),
+ ACK(Stomp.Commands.ACK),
+ NACK(Stomp.Commands.NACK),
+ STOMP(Stomp.Commands.STOMP),
+ CONNECTED(Stomp.Responses.CONNECTED),
+ ERROR(Stomp.Responses.ERROR),
+ MESSAGE(Stomp.Responses.MESSAGE),
+ RECEIPT(Stomp.Responses.RECEIPT);
+
+ private String type;
+
+ CommandType(String command) {
+ type = command;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+
+ public static CommandType getType(String command) {
+ switch (command) {
--- End diff --
There are serious performance implications on this, right?
you're using getType on the hot path... (on Every frame creation). What happened to that code that would inspect just a few bytes of the frame and create the StompFrame fast... this is a major regression here.
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1960
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
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/1960#discussion_r176078341
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompSession.java ---
@@ -154,12 +154,16 @@ public int sendMessage(MessageReference ref,
encoder.encode(buffer, bodySize);
encoder.close();
} else {
- buffer = coreMessage.getReadOnlyBodyBuffer();
+ if (Boolean.TRUE.equals(serverMessage.getBooleanProperty(Message.HDR_LARGE_COMPRESSED))) {
+ buffer = coreMessage.getBodyBuffer();
--- End diff --
You should always use the readOnlyBodyBuffer...
Why did you need the writeable body buffer here? it's dangerous!
---
[GitHub] activemq-artemis pull request #1960: ARTEMIS-406 STOMP acknowledgements shou...
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/1960#discussion_r176109013
--- Diff: artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompTransaction.java ---
@@ -0,0 +1,82 @@
+/*
+ * 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.protocol.stomp;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.server.ServerSession;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Control's stomp transaction processing
+ * it goes with stomp connections.
+ * it stores acks, sends, nacks
+ * during commit it applies those to core sessions.
+ * because each stomp connection uses only one session
+ * we can't rely on core session to manage multiple
+ * tx's.
+ */
+public class StompTransaction {
+
+ private List<StompAck> acks = new ArrayList<>();
--- End diff --
Transactions in Artemis are usually done right away...
The operation on the journal is stored right away... then a pending commit is in place.
the thing that if you duplicate Transaction functionality from the session, you won't inherit other features such as timeouts.. etc...
If you need to support this, I would do it properly.. even if it requires some light refactoring on how transactions are stored. (I believe you could use the XID to get the transaction.. as a matter of fact the previous version had some code before such as manager.getTransactionI()).
later on when users complain about a timed out transactions, you will have a better chance of implementing through the ResourceManager that is already in place.
---
[GitHub] activemq-artemis issue #1960: ARTEMIS-406 STOMP acknowledgements should supp...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on the issue:
https://github.com/apache/activemq-artemis/pull/1960
I'm closing it for now as we need some discussion.
---