ibessonov commented on code in PR #1869:
URL: https://github.com/apache/ignite-3/pull/1869#discussion_r1159645506
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##########
@@ -151,15 +155,22 @@ private DistributionZoneManager
prepareDistributionZoneManager() {
keyValueStorage = spy(new SimpleInMemoryKeyValueStorage("test"));
- MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage);
+ MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage, mock(ClusterTimeImpl.class));
RaftGroupService metaStorageService = mock(RaftGroupService.class);
+ HybridTimestampMessage mockTsMessage =
mock(HybridTimestampMessage.class);
Review Comment:
Can't you just build it?
Man, I wish massage factories were static, it's so annoying
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##########
@@ -151,15 +155,22 @@ private DistributionZoneManager
prepareDistributionZoneManager() {
keyValueStorage = spy(new SimpleInMemoryKeyValueStorage("test"));
- MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage);
+ MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage, mock(ClusterTimeImpl.class));
RaftGroupService metaStorageService = mock(RaftGroupService.class);
+ HybridTimestampMessage mockTsMessage =
mock(HybridTimestampMessage.class);
+ when(mockTsMessage.asHybridTimestamp()).thenReturn(new
HybridTimestamp(10, 10));
+
// Delegate directly to listener.
lenient().doAnswer(
invocationClose -> {
Review Comment:
I wonder, what's the "close" in this `invocationClose`. Closure?
Anyway, I have a strong feeling of deja vu, almost as if this code is
copy-pasted multiple times. Can you please check that?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -198,7 +199,7 @@ public void setUp() {
keyValueStorage = spy(new SimpleInMemoryKeyValueStorage("test"));
- MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage);
+ MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage, mock(ClusterTimeImpl.class));
Review Comment:
Will passing a real instance make a difference?
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageMultipleNodesTest.java:
##########
@@ -118,11 +123,12 @@ private static class Node {
Path basePath = dataPath.resolve(name());
+ HybridClockImpl clock = new HybridClockImpl();
Review Comment:
Same here
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/command/MetaStorageWriteCommand.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.command;
+
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.raft.Command;
+import org.apache.ignite.internal.raft.WriteCommand;
+import org.apache.ignite.network.annotations.WithSetter;
+
+/** Base meta storage write command. */
+public interface MetaStorageWriteCommand extends WriteCommand {
+ /**
+ * Returns time on the initiator node.
+ */
+ HybridTimestampMessage initiatorTime();
+
+ /**
+ * This is a dirty hack. This time is set by the leader node to
disseminate new safe time across
+ * followers and learners. Leader of the ms group reads {@link
#initiatorTime()}, adjusts its clock
+ * and sets safeTime as {@link HybridClock#now()} as safeTime here. This
must be done before
+ * command is saved into the Raft log (see {@link
org.apache.ignite.internal.raft.service.RaftGroupListener#onBeforeApply(Command)}.
+ */
+ @WithSetter
+ HybridTimestampMessage safeTime();
+
+ /**
+ * Setter for the safeTime field.
+ */
+ default void safeTime(HybridTimestampMessage safeTime) {
Review Comment:
What's the reason to have a default implementation here?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/command/MetastorageCommandsMessageGroup.java:
##########
@@ -84,4 +84,10 @@ public interface MetastorageCommandsMessageGroup {
/** Message type for {@link CloseAllCursorsCommand}. */
short CLOSE_ALL_CURSORS = 64;
+
+ /** Message type for {@link HybridTimestampMessage}. */
+ short HYBRID_TS = 65;
Review Comment:
Maybe it would make sense to move these ids into a `7x` numeration.
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -66,6 +76,11 @@ class MetaStorageWriteHandler {
boolean handleWriteCommand(CommandClosure<WriteCommand> clo) {
WriteCommand command = clo.command();
+ if (command instanceof MetaStorageWriteCommand) {
Review Comment:
No-no-no, please update timestamp _after_ you finished processing :)
Otherwise you'll have data races
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageManagerImplTest.java:
##########
@@ -91,7 +91,8 @@ void setUp(TestInfo testInfo, @InjectConfiguration
RaftConfiguration raftConfigu
clusterService = clusterService(testInfo, addr.port(), new
StaticNodeFinder(List.of(addr)));
- raftManager = new Loza(clusterService, raftConfiguration,
workDir.resolve("loza"), new HybridClockImpl());
+ HybridClockImpl clock = new HybridClockImpl();
Review Comment:
Please keep types clean. Don't commit this suggestion blindly, there's
probably no import for this class.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##########
@@ -151,15 +155,22 @@ private DistributionZoneManager
prepareDistributionZoneManager() {
keyValueStorage = spy(new SimpleInMemoryKeyValueStorage("test"));
- MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage);
+ MetaStorageListener metaStorageListener = new
MetaStorageListener(keyValueStorage, mock(ClusterTimeImpl.class));
RaftGroupService metaStorageService = mock(RaftGroupService.class);
+ HybridTimestampMessage mockTsMessage =
mock(HybridTimestampMessage.class);
+ when(mockTsMessage.asHybridTimestamp()).thenReturn(new
HybridTimestamp(10, 10));
+
// Delegate directly to listener.
lenient().doAnswer(
invocationClose -> {
Command cmd = invocationClose.getArgument(0);
+ if (cmd instanceof MetaStorageWriteCommand) {
Review Comment:
I'm sure you had a reason to do it this way. What happens when someone
forgets to make this propagation in test? NPE?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/command/MetaStorageWriteCommand.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.command;
+
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.raft.Command;
+import org.apache.ignite.internal.raft.WriteCommand;
+import org.apache.ignite.network.annotations.WithSetter;
+
+/** Base meta storage write command. */
+public interface MetaStorageWriteCommand extends WriteCommand {
+ /**
+ * Returns time on the initiator node.
+ */
+ HybridTimestampMessage initiatorTime();
Review Comment:
We don't need to have this property in the future, because it's the
attribute of action request, right? I would prefer you to address this in the
comment. Maybe even deprecating it straight away
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/command/SyncTimeCommand.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.command;
+
+import org.apache.ignite.internal.raft.WriteCommand;
+import org.apache.ignite.network.annotations.Transferable;
+
+/**
+ * Command that initiates idle safe time synchronization.
+ */
+@Transferable(MetastorageCommandsMessageGroup.SYNC_TIME)
+public interface SyncTimeCommand extends WriteCommand {
+ /** New safe time. */
+ HybridTimestampMessage safeTime();
Review Comment:
Does not-inheriting it from metastorage write message make code simpler?
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageManagerImplTest.java:
##########
@@ -91,7 +91,8 @@ void setUp(TestInfo testInfo, @InjectConfiguration
RaftConfiguration raftConfigu
clusterService = clusterService(testInfo, addr.port(), new
StaticNodeFinder(List.of(addr)));
- raftManager = new Loza(clusterService, raftConfiguration,
workDir.resolve("loza"), new HybridClockImpl());
+ HybridClockImpl clock = new HybridClockImpl();
Review Comment:
```suggestion
HybridClock clock = new HybridClockImpl();
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/command/HybridTimestampMessage.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.command;
+
+import java.io.Serializable;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.network.NetworkMessage;
+import org.apache.ignite.network.annotations.Transferable;
+
+/** Message with a {@link HybridTimestamp}. */
+@Transferable(MetastorageCommandsMessageGroup.HYBRID_TS)
+public interface HybridTimestampMessage extends NetworkMessage, Serializable {
Review Comment:
Is this a copy-paste of
`org.apache.ignite.internal.replicator.command.HybridTimestampMessage`?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/MetaStorageWriteHandler.java:
##########
@@ -234,4 +253,18 @@ private static RevisionCondition.Type
toRevisionConditionType(ConditionType type
throw new IllegalArgumentException("Unexpected revision
condition type " + type);
}
}
+
+ void beforeApply(Command command) {
Review Comment:
I'm trying to wrap my head around this method.
First of all, it is executed on each node in the cluster, correct? If it is,
then it feels very dangerous, because it may result in different command times
at different nodes.
Please explain it thoroughly, I feel like there's a bug here.
--
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]
