You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/11/24 14:40:20 UTC
[GitHub] [ignite-3] rpuch opened a new pull request, #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
rpuch opened a new pull request, #1376:
URL: https://github.com/apache/ignite-3/pull/1376
https://issues.apache.org/jira/browse/IGNITE-18241
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1032364985
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.cluster.management.topology;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.internal.util.ByteUtils.fromBytes;
+import static org.apache.ignite.internal.util.ByteUtils.toBytes;
+
+import java.util.Collection;
+import java.util.Set;
+import org.apache.ignite.internal.cluster.management.raft.ClusterStateStorage;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * Implementation of {@link LogicalTopology}.
+ */
+public class LogicalTopologyImpl implements LogicalTopology {
Review Comment:
I was just curious, it's fine
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1032235424
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.cluster.management.topology;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.internal.util.ByteUtils.fromBytes;
+import static org.apache.ignite.internal.util.ByteUtils.toBytes;
+
+import java.util.Collection;
+import java.util.Set;
+import org.apache.ignite.internal.cluster.management.raft.ClusterStateStorage;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * Implementation of {@link LogicalTopology}.
+ */
+public class LogicalTopologyImpl implements LogicalTopology {
Review Comment:
Before the previous PR, we didn't; before this PR, we did. This caching looks like an optimization and it's not clear whether it's needed at all (all operations with logical topology seem to be pretty rare). But it requires to complicate the design a bit (flush the cache after restoring from a snapshot), so I decided that it should be omitted for now.
WDYT?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1032367929
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -245,11 +246,16 @@ private List<IgniteComponent> startPartialNode(
var txManager = new TxManagerImpl(replicaService, lockManager, hybridClock);
+ var clusterStateStorage = new RocksDbClusterStateStorage(dir.resolve("cmg"));
+
+ LogicalTopologyImpl logicalTopologyService = new LogicalTopologyImpl(clusterStateStorage);
Review Comment:
We can use `var` here as well. But this is not important
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1031611247
##########
modules/rest/openapi/openapi.yaml:
##########
@@ -6,7 +6,7 @@ info:
license:
name: Apache 2.0
url: https://ignite.apache.org
- version: 3.0.0-SNAPSHOT
+ version: 3.0.0-alpha
Review Comment:
So, should we commit this or is it a bug?
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/MockNode.java:
##########
@@ -76,11 +77,16 @@ private void init(int port) throws IOException {
Loza raftManager = new Loza(clusterService, null, workDir, new HybridClockImpl());
+ RocksDbClusterStateStorage clusterStateStorage = new RocksDbClusterStateStorage(workDir.resolve("cmg"));
Review Comment:
we can use `var` here
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.cluster.management.topology;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.internal.util.ByteUtils.fromBytes;
+import static org.apache.ignite.internal.util.ByteUtils.toBytes;
+
+import java.util.Collection;
+import java.util.Set;
+import org.apache.ignite.internal.cluster.management.raft.ClusterStateStorage;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * Implementation of {@link LogicalTopology}.
+ */
+public class LogicalTopologyImpl implements LogicalTopology {
Review Comment:
Didn't we use to cache the current topology snapshot?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftGroupListener.java:
##########
@@ -54,11 +55,17 @@ public class CmgRaftGroupListener implements RaftGroupListener {
private final RaftStorageManager storage;
+ private final LogicalTopology logicalTopologyService;
+
private final ValidationManager validationManager;
- public CmgRaftGroupListener(ClusterStateStorage storage) {
+ /**
+ * Creates a new instance.
+ */
+ public CmgRaftGroupListener(ClusterStateStorage storage, LogicalTopology logicalTopologyService) {
this.storage = new RaftStorageManager(storage);
- this.validationManager = new ValidationManager(this.storage);
+ this.logicalTopologyService = logicalTopologyService;
Review Comment:
let's rename the field and the parameter as well
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill merged pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
tkalkirill merged PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1032358899
##########
modules/rest/openapi/openapi.yaml:
##########
@@ -6,7 +6,7 @@ info:
license:
name: Apache 2.0
url: https://ignite.apache.org
- version: 3.0.0-SNAPSHOT
+ version: 3.0.0-alpha
Review Comment:
Oops, this thing again. Fixed it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1376: IGNITE-18241 Introduce support code for LogicalTopologyService
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #1376:
URL: https://github.com/apache/ignite-3/pull/1376#discussion_r1032235424
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.cluster.management.topology;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.ignite.internal.util.ByteUtils.fromBytes;
+import static org.apache.ignite.internal.util.ByteUtils.toBytes;
+
+import java.util.Collection;
+import java.util.Set;
+import org.apache.ignite.internal.cluster.management.raft.ClusterStateStorage;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * Implementation of {@link LogicalTopology}.
+ */
+public class LogicalTopologyImpl implements LogicalTopology {
Review Comment:
Before the previous PR, we didn't; before this PR, we did. This caching looks like an optimization and it's not clear whether it's needed at all (all operations with logical topology seem to be pretty rare).
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org