sanpwc commented on code in PR #3725:
URL: https://github.com/apache/ignite-3/pull/3725#discussion_r1598782469
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -278,4 +309,50 @@ boolean beforeApply(Command command) {
return false;
}
+
+ private static class IdempotentCommandCache {
Review Comment:
I don't really like naming here. The map itself is a cache, but not the
value. I'd rather rename to IdempotentCommandCacheEntry or similar.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/CommandIdGenerator.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.metastorage.impl;
+
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+import org.apache.ignite.internal.metastorage.CommandId;
+import org.apache.ignite.internal.metastorage.dsl.MetaStorageMessagesFactory;
+
+/**
+ * Generates the command ids.
+ */
+public class CommandIdGenerator {
+ private static final MetaStorageMessagesFactory MSG_FACTORY = new
MetaStorageMessagesFactory();
+
+ /** Supplies nodeId for transactionId generation. */
Review Comment:
TransactionId?
Anyway, why it's a supplier? Do you mean that we don't know nodeId on
generator creation?
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageServicePersistenceTest.java:
##########
@@ -76,7 +76,8 @@ public void beforeFollowerStop(RaftGroupService service,
RaftServer server) thro
var clusterTime = new ClusterTimeImpl(followerNode.name(), new
IgniteSpinBusyLock(), new HybridClockImpl());
- metaStorage = new MetaStorageServiceImpl(followerNode.name(), service,
new IgniteSpinBusyLock(), clusterTime);
+ metaStorage = new MetaStorageServiceImpl(followerNode.name(), service,
new IgniteSpinBusyLock(), clusterTime,
Review Comment:
I'd rather put one param at a line here. It's 4 of them already.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -278,4 +309,50 @@ boolean beforeApply(Command command) {
return false;
}
+
+ private static class IdempotentCommandCache {
+ @Nullable
+ final Serializable result;
+
+ final HybridTimestamp commandStartTime;
+
+ IdempotentCommandCache(@Nullable Serializable result, HybridTimestamp
commandStartTime) {
+ this.result = result;
+ this.commandStartTime = commandStartTime;
+ }
+ }
+
+ private class ResultCachingClosure implements CommandClosure<WriteCommand>
{
+ CommandClosure<WriteCommand> closure;
+
+ ResultCachingClosure(CommandClosure<WriteCommand> closure) {
+ this.closure = closure;
+
+ assert closure.command() instanceof IdempotentCommand;
+ }
+
+ @Override
+ public long index() {
+ return closure.index();
+ }
+
+ @Override
+ public long term() {
+ return closure.term();
+ }
+
+ @Override
+ public WriteCommand command() {
+ return closure.command();
+ }
+
+ @Override
+ public void result(@Nullable Serializable res) {
+ IdempotentCommand command = (IdempotentCommand) closure.command();
+
+ idempotentCommandCache.put(command.id(), new
IdempotentCommandCache(res, command.initiatorTime()));
Review Comment:
Instead of command.initiatorTime I'd rather use System.currentTimeMillies
here.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -72,6 +80,29 @@ public class MetaStorageWriteHandler {
void handleWriteCommand(CommandClosure<WriteCommand> clo) {
WriteCommand command = clo.command();
+ CommandClosure<WriteCommand> resultClosure;
+
+ if (command instanceof IdempotentCommand) {
+ CommandId commandId = ((IdempotentCommand) command).id();
+ IdempotentCommandCache cache =
idempotentCommandCache.get(commandId);
Review Comment:
The var naming is confusing, please check another comment for more details.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -278,4 +309,50 @@ boolean beforeApply(Command command) {
return false;
}
+
+ private static class IdempotentCommandCache {
+ @Nullable
+ final Serializable result;
+
+ final HybridTimestamp commandStartTime;
+
+ IdempotentCommandCache(@Nullable Serializable result, HybridTimestamp
commandStartTime) {
+ this.result = result;
+ this.commandStartTime = commandStartTime;
+ }
+ }
+
+ private class ResultCachingClosure implements CommandClosure<WriteCommand>
{
+ CommandClosure<WriteCommand> closure;
+
+ ResultCachingClosure(CommandClosure<WriteCommand> closure) {
+ this.closure = closure;
+
+ assert closure.command() instanceof IdempotentCommand;
+ }
+
+ @Override
+ public long index() {
+ return closure.index();
+ }
+
+ @Override
+ public long term() {
+ return closure.term();
+ }
+
+ @Override
+ public WriteCommand command() {
+ return closure.command();
+ }
+
+ @Override
+ public void result(@Nullable Serializable res) {
+ IdempotentCommand command = (IdempotentCommand) closure.command();
+
+ idempotentCommandCache.put(command.id(), new
IdempotentCommandCache(res, command.initiatorTime()));
Review Comment:
So, we will put an exception as cached result. I don't think that it's
correct.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -278,4 +309,50 @@ boolean beforeApply(Command command) {
return false;
}
+
+ private static class IdempotentCommandCache {
+ @Nullable
+ final Serializable result;
+
+ final HybridTimestamp commandStartTime;
Review Comment:
I don't think that we need HybridTimestamp here. Common long with current
time should be enough. WDYT?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -278,4 +309,50 @@ boolean beforeApply(Command command) {
return false;
}
+
+ private static class IdempotentCommandCache {
+ @Nullable
Review Comment:
Is it really nullable now?
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/CommandId.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.metastorage;
+
+import java.util.UUID;
+import org.apache.ignite.internal.metastorage.dsl.MetaStorageMessageGroup;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.apache.ignite.internal.network.annotations.Transferable;
+
+/**
+ * Command id.
Review Comment:
Could you please add few more words about uniquness guarantess etc?
--
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]
