sashapolo commented on code in PR #6089: URL: https://github.com/apache/ignite-3/pull/6089#discussion_r2159992382
########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItEnabledColocationHomogeneityTest.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.runner.app; + +import static org.apache.ignite.internal.lang.IgniteSystemProperties.COLOCATION_FEATURE_FLAG; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause; + +import org.apache.ignite.internal.BaseIgniteRestartTest; +import org.apache.ignite.internal.cluster.management.raft.JoinDeniedException; +import org.apache.ignite.internal.lang.IgniteStringFormatter; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Tests to check enabled colocation homogeneity within node join validation. + */ +@SuppressWarnings("ThrowableNotThrown") +public class ItEnabledColocationHomogeneityTest extends BaseIgniteRestartTest { Review Comment: I would suggest to add a `tearDown` method that will restore the colocation flag to its original value. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItEnabledColocationHomogeneityTest.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.runner.app; + +import static org.apache.ignite.internal.lang.IgniteSystemProperties.COLOCATION_FEATURE_FLAG; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause; + +import org.apache.ignite.internal.BaseIgniteRestartTest; +import org.apache.ignite.internal.cluster.management.raft.JoinDeniedException; +import org.apache.ignite.internal.lang.IgniteStringFormatter; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Tests to check enabled colocation homogeneity within node join validation. + */ +@SuppressWarnings("ThrowableNotThrown") +public class ItEnabledColocationHomogeneityTest extends BaseIgniteRestartTest { + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testJoinDeniedIsThrownInCaseOfIncompatibleColocationModes(boolean colocationEnabled) { + System.setProperty(COLOCATION_FEATURE_FLAG, Boolean.toString(colocationEnabled)); + startNode(0); + + System.setProperty(COLOCATION_FEATURE_FLAG, Boolean.toString(!colocationEnabled)); + assertThrowsWithCause( + () -> startNode(1), + JoinDeniedException.class, + IgniteStringFormatter.format("Colocation enabled mode does not match. Joining node colocation mode is: {}," + + " cluster colocation mode is: {}", !colocationEnabled, colocationEnabled) + ); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testJoinDeniedIsNotThrownInCaseOfIncompatibleColocationModes(boolean colocationEnabled) { Review Comment: ```suggestion public void testJoinDeniedIsNotThrownInCaseOfCompatibleColocationModes(boolean colocationEnabled) { ``` ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationManager.java: ########## @@ -112,13 +113,28 @@ protected ValidationResult validateNode( "Cluster tags do not match. Cluster tag: %s, cluster tag stored in CMG: %s", clusterTag, state.clusterTag() )); + } else if (!isColocationEnabledMatched(isColocationEnabled(node))) { + return ValidationResult.errorResult(String.format( + "Colocation enabled mode does not match. Joining node colocation mode is: %s, cluster colocation mode is: %s", + isColocationEnabled(node), + isColocationEnabled(logicalTopology.getLogicalTopology().nodes().iterator().next()) + )); } else { putValidatedNode(node); return ValidationResult.successfulResult(); } } + private static boolean isColocationEnabled(LogicalNode node) { + return Boolean.parseBoolean(node.systemAttributes().get(COLOCATION_FEATURE_FLAG)); + } + + private boolean isColocationEnabledMatched(boolean joiningNodeColocationEnabled) { + return logicalTopology.getLogicalTopology().nodes().isEmpty() Review Comment: Please extract `logicalTopology.getLogicalTopology().nodes()` into a variable ########## modules/runner/src/main/java/org/apache/ignite/internal/app/NodePropertiesImpl.java: ########## @@ -105,4 +108,9 @@ public boolean colocationEnabled() { public CompletableFuture<Void> stopAsync(ComponentContext componentContext) { return nullCompletedFuture(); } + + @Override + public Map<String, String> nodeAttributes() { + return Map.of(COLOCATION_FEATURE_FLAG, Boolean.toString(colocationEnabled())); Review Comment: It's a bit misleading, that you are using the system property as a node attribute name. Can we at least have a different java constant for this? ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationManager.java: ########## @@ -112,13 +113,28 @@ protected ValidationResult validateNode( "Cluster tags do not match. Cluster tag: %s, cluster tag stored in CMG: %s", clusterTag, state.clusterTag() )); + } else if (!isColocationEnabledMatched(isColocationEnabled(node))) { + return ValidationResult.errorResult(String.format( + "Colocation enabled mode does not match. Joining node colocation mode is: %s, cluster colocation mode is: %s", + isColocationEnabled(node), + isColocationEnabled(logicalTopology.getLogicalTopology().nodes().iterator().next()) Review Comment: Each `logicalTopology.getLogicalTopology()` is a storage read, please think about reducing the amount of these calls ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationManager.java: ########## @@ -112,13 +113,28 @@ protected ValidationResult validateNode( "Cluster tags do not match. Cluster tag: %s, cluster tag stored in CMG: %s", clusterTag, state.clusterTag() )); + } else if (!isColocationEnabledMatched(isColocationEnabled(node))) { + return ValidationResult.errorResult(String.format( + "Colocation enabled mode does not match. Joining node colocation mode is: %s, cluster colocation mode is: %s", + isColocationEnabled(node), + isColocationEnabled(logicalTopology.getLogicalTopology().nodes().iterator().next()) + )); } else { putValidatedNode(node); return ValidationResult.successfulResult(); } } + private static boolean isColocationEnabled(LogicalNode node) { Review Comment: Can we unify these two methods into one and have something like: `boolean joiningNodeMatchesColocationMode` ? -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
