sanpwc commented on code in PR #2877:
URL: https://github.com/apache/ignite-3/pull/2877#discussion_r1413439293
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxMessageGroup.java:
##########
@@ -58,4 +58,17 @@ public class TxMessageGroup {
* Message type for {@link TxRecoveryMessage}.
*/
public static final short TX_RECOVERY_MSG = 6;
+
+ /**
+ * Message type for {@link LockReleaseMessage}.
+ */
+ public static final short TX_UNLOCK_MSG = 7;
+ /**
Review Comment:
Empty line is missing. Same below.
##########
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeTxManager.java:
##########
@@ -190,6 +191,12 @@ public CompletableFuture<Void> cleanup(
return completedFuture(null);
}
+ @Override
+ public CompletableFuture<Void> unlock(Collection<TablePartitionId>
partitions, boolean commit,
Review Comment:
Minor: I'd rather have one param per raw.
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/impl/DummyMessagingService.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.ignite.internal.table.impl;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.network.AbstractMessagingService;
+import org.apache.ignite.network.ChannelType;
+import org.apache.ignite.network.ClusterNode;
+import org.apache.ignite.network.NetworkMessage;
+
+/**
+ * Dummy messaging service for tests purposes.
+ * It does not provide any messaging functionality, but allows to trigger
events.
+ */
+public class DummyMessagingService extends AbstractMessagingService {
+ private final String localNodeName;
+
+ private final AtomicLong correlationIdGenerator = new AtomicLong();
+
+ public DummyMessagingService(String localNodeName) {
+ this.localNodeName = localNodeName;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void weakSend(ClusterNode recipient, NetworkMessage msg) {
+ throw new AssertionError("Not implemented yet");
Review Comment:
I'd rather use `UnsupportedOperationException`, up to you.
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/impl/DummyMessagingService.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.ignite.internal.table.impl;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.network.AbstractMessagingService;
+import org.apache.ignite.network.ChannelType;
+import org.apache.ignite.network.ClusterNode;
+import org.apache.ignite.network.NetworkMessage;
+
+/**
+ * Dummy messaging service for tests purposes.
+ * It does not provide any messaging functionality, but allows to trigger
events.
+ */
+public class DummyMessagingService extends AbstractMessagingService {
Review Comment:
Are we going to use it somewhere besides DummyInternalTable?
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ItTxDistributedCleanupRecoveryTest.java:
##########
@@ -155,47 +66,19 @@ public CompletableFuture<ReplicaResult>
invoke(ReplicaRequest request, String se
accounts = txTestCluster.startTable(ACC_TABLE_NAME, ACC_TABLE_ID,
ACCOUNTS_SCHEMA);
customers = txTestCluster.startTable(CUST_TABLE_NAME, CUST_TABLE_ID,
CUSTOMERS_SCHEMA);
- log.info("Tables have been started");
- }
-
-
- @Test
- @Override
- public void testDeleteUpsertCommit() throws TransactionException {
- // The value of 6 is higher than the default retry count.
- // So we should give up retrying and crash.
- setDefaultRetryCount(6);
-
- assertThrows(TransactionException.class, () ->
deleteUpsert().commit());
- }
-
- @Test
Review Comment:
What's the point of removing such tests?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/LockReleaseMessageResponse.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.ignite.internal.tx.message;
+
+import org.apache.ignite.internal.replicator.message.TimestampAware;
+import org.apache.ignite.network.annotations.Transferable;
+
+/**
+ * Release transaction locks message response.
+ */
+@Transferable(TxMessageGroup.TX_UNLOCK_MSG_RESPONSE)
+public interface LockReleaseMessageResponse extends TimestampAware {
Review Comment:
Formally speaking, this (and all other unlock related) message may not be
timestamp aware because commitTimestamp was already calculated. So, what's the
point in "extends TimestampAware": is it just common generalization? (BTW, if
it is I still believe that it's fine)
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/LockReleaseMessage.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.ignite.internal.tx.message;
+
+import static
org.apache.ignite.internal.hlc.HybridTimestamp.nullableHybridTimestamp;
+
+import java.util.Collection;
+import java.util.UUID;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.replicator.ReplicationGroupId;
+import org.apache.ignite.internal.replicator.message.TimestampAware;
+import org.apache.ignite.network.NetworkMessage;
+import org.apache.ignite.network.annotations.Marshallable;
+import org.apache.ignite.network.annotations.Transferable;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Release transaction locks message.
+ */
+@Transferable(TxMessageGroup.TX_UNLOCK_MSG)
+public interface LockReleaseMessage extends NetworkMessage, TimestampAware {
Review Comment:
TimestampAware extends NetworkMessage itself.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/LockReleaseMessage.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.ignite.internal.tx.message;
+
+import static
org.apache.ignite.internal.hlc.HybridTimestamp.nullableHybridTimestamp;
+
+import java.util.Collection;
+import java.util.UUID;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.replicator.ReplicationGroupId;
+import org.apache.ignite.internal.replicator.message.TimestampAware;
+import org.apache.ignite.network.NetworkMessage;
+import org.apache.ignite.network.annotations.Marshallable;
+import org.apache.ignite.network.annotations.Transferable;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Release transaction locks message.
+ */
+@Transferable(TxMessageGroup.TX_UNLOCK_MSG)
+public interface LockReleaseMessage extends NetworkMessage, TimestampAware {
+ /**
+ * Gets a transaction id to resolve.
+ *
+ * @return Transaction id.
+ */
+ UUID txId();
+
+ /**
+ * Returns replication groups aggregated by expected primary replica nodes.
+ * Null when this message is sent at recovery.
+ *
+ * @return Replication groups aggregated by expected primary replica nodes.
+ */
+ @Marshallable
+ @Nullable
+ Collection<ReplicationGroupId> groups();
Review Comment:
It's a tricky part.
1. AFAIK SE asked not to use @Marshallable if it's possible.
2. We may consider using TablePartitionId instead of ReplicationGroupId,
because transactions cover only tables.
All in an several months ago, I've tried following options
1. Works fine.
```
@Marshallable
Collection<ReplicationGroupId> groups();
```
2. Doesn't work.
```
@Marshallable
Collection<TablePartitionId> groups();
```
3. Doesn't work.
`Collection<TablePartitionId> groups();`
@ibessonov told me that we should use `TableParitionIdMessage`, however I
can't find such class not.
Wondering whether anything have changed?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/LockReleaseMessageErrorResponse.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.ignite.internal.tx.message;
+
+import org.apache.ignite.network.annotations.Marshallable;
+import org.apache.ignite.network.annotations.Transferable;
+
+/**
+ * Release transaction locks message response.
+ */
+@Transferable(TxMessageGroup.TX_UNLOCK_MSG_ERR_RESPONSE)
+public interface LockReleaseMessageErrorResponse extends
LockReleaseMessageResponse {
+
+ /**
+ * Returns a {@link Throwable} that was thrown during handling a lock
release message.
+ *
+ * @return {@link Throwable} that was thrown during handling a lock
release message.
+ */
+ @Marshallable
Review Comment:
@ibessonov Is that right?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -198,6 +203,8 @@ public TxManagerImpl(
new NamedThreadFactory("tx-async-cleanup", LOG));
orphanDetector = new OrphanDetector(clusterService.topologyService(),
replicaService, placementDriver, lockManager, clock);
+
+ txUnlockManager = new TxUnlockManager(clusterService, this,
lockManager, placementDriver, clock);
Review Comment:
Why we need the TxUnlockManager? Especially because we already have
LockManager with all that acquire and release operations. From my point of view
adding TxUnlockManager will bring event more mess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
