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 2021/10/07 14:34:35 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request #387: IGNITE-15707 Ability to configure data regions for tables
ibessonov opened a new pull request #387:
URL: https://github.com/apache/ignite-3/pull/387
https://issues.apache.org/jira/browse/IGNITE-15707
--
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] ibessonov merged pull request #387: IGNITE-15707 Ability to configure data regions for tables
Posted by GitBox <gi...@apache.org>.
ibessonov merged pull request #387:
URL: https://github.com/apache/ignite-3/pull/387
--
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 change in pull request #387: IGNITE-15707 Ability to configure data regions for tables
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #387:
URL: https://github.com/apache/ignite-3/pull/387#discussion_r728885574
##########
File path: modules/configuration-api/src/main/java/org/apache/ignite/configuration/validation/Except.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.configuration.validation;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+/**
+ * Signifies that current {@link NamedConfigValue} can't have element with provided names.
Review comment:
```suggestion
* Signifies that a {@link NamedConfigValue} can't have elements with provided names.
```
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -30,6 +34,8 @@
import org.apache.ignite.internal.schema.definition.builder.TableSchemaBuilderImpl;
import org.apache.ignite.schema.definition.ColumnDefinition;
+import static org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.DEFAULT_DATA_REGION_NAME;
+
/**
* Table schema configuration validator implementation.
Review comment:
Do I understand correctly that this class is not used in the production code? Is that intended?
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -57,10 +64,68 @@
}
catch (IllegalArgumentException e) {
ctx.addIssue(new ValidationIssue("Validator works success by key " + ctx.currentKey() + ". Found "
- + view.columns().size() + " columns"));
+ + newTable.columns().size() + " columns"));
}
+
+ validateDataRegion(oldTables == null ? null : oldTables.get(tableName), newTable, ctx);
}
+ }
+
+ /**
+ * Validate data region validity.
Review comment:
I think it would be better to rephrase this sentence =)
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -148,8 +151,8 @@
/** Resolver that resolves a network address to node id. */
private final Function<NetworkAddress, String> netAddrResolver;
- /** Default data region instance. */
- private DataRegion defaultDataRegion;
+ /** Data region instances. */
+ private Map<String, DataRegion> dataRegions = new ConcurrentHashMap<>();
Review comment:
```suggestion
private final Map<String, DataRegion> dataRegions = new ConcurrentHashMap<>();
```
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -57,10 +64,68 @@
}
catch (IllegalArgumentException e) {
ctx.addIssue(new ValidationIssue("Validator works success by key " + ctx.currentKey() + ". Found "
- + view.columns().size() + " columns"));
+ + newTable.columns().size() + " columns"));
}
+
+ validateDataRegion(oldTables == null ? null : oldTables.get(tableName), newTable, ctx);
}
+ }
+
+ /**
+ * Validate data region validity.
+ *
+ * @param oldTable Previous configuration, maybe {@code null}.
+ * @param newTable New configuration.
+ * @param ctx Validation context.
+ */
+ // TODO Rename isn't supported right now.
+ private void validateDataRegion(TableView oldTable, TableView newTable, ValidationContext<?> ctx) {
Review comment:
```suggestion
private void validateDataRegion(@Nullable TableView oldTable, TableView newTable, ValidationContext<?> ctx) {
```
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -39,13 +45,14 @@
/** {@inheritDoc} */
@Override public void validate(TableValidator annotation, ValidationContext<NamedListView<TableView>> ctx) {
- NamedListView<TableView> list = ctx.getNewValue();
-
- for (String key : list.namedListKeys()) {
- TableView view = list.get(key);
-
+ NamedListView<TableView> oldTables = ctx.getOldValue();
+ NamedListView<TableView> newTables = ctx.getNewValue();
+
+ for (String tableName : newTables.namedListKeys()) {
Review comment:
Not related to this PR, but line 63 is a "little bit" too long, can you fix it please?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/validation/ExceptValidator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.configuration.validation;
+
+import java.util.List;
+import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.configuration.validation.Except;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.configuration.validation.Validator;
+
+/**
+ * {@link Validator} implementation for the {@link Except} annotation.
+ */
+public class ExceptValidator implements Validator<Except, NamedListView<?>> {
+ /** {@inheritDoc} */
+ @Override public void validate(Except annotation, ValidationContext<NamedListView<?>> ctx) {
+ NamedListView<?> nameList = ctx.getNewValue();
+
+ List<String> actualNames = nameList.namedListKeys();
+
+ for (String exceptedName : annotation.value()) {
+ if (actualNames.contains(exceptedName)) {
+ String message = "'" + ctx.currentKey() + "' configuration must not contain element named '" +
Review comment:
```suggestion
String message = "'" + ctx.currentKey() + "' configuration must not contain elements named '" +
```
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -324,7 +327,9 @@ public TableManager(
}
});
- this.defaultDataRegion = engine.createDataRegion(dataStorageCfg.defaultRegion());
+ DataRegion defaultDataRegion = engine.createDataRegion(dataStorageCfg.defaultRegion());
+
+ dataRegions.put(DEFAULT_DATA_REGION_NAME, defaultDataRegion);
Review comment:
shall we initialize the default data region lazily as the other data regions?
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/TableValidatorImplTest.java
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.schema.configuration;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.configuration.schemas.store.DataStorageView;
+import org.apache.ignite.configuration.schemas.table.TableView;
+import org.apache.ignite.configuration.schemas.table.TablesConfiguration;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/** */
+@ExtendWith(ConfigurationExtension.class)
+public class TableValidatorImplTest {
Review comment:
shall we also test the happy case?
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/TableValidatorImplTest.java
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.schema.configuration;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.configuration.schemas.store.DataStorageView;
+import org.apache.ignite.configuration.schemas.table.TableView;
+import org.apache.ignite.configuration.schemas.table.TablesConfiguration;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/** */
+@ExtendWith(ConfigurationExtension.class)
+public class TableValidatorImplTest {
+ /** Basic table configuration to mutate and then validate. */
+ @InjectConfiguration("mock.tables.table {\n" +
+ " name = schema.table,\n" +
+ " columns.0 {name = id, type.type = STRING, nullable = true},\n" +
+ " primaryKey {columns = [id], affinityColumns = [id]}\n" +
+ "}")
+ private TablesConfiguration tableCfg;
+
+ /** Tests that validater catches nonexistent data regions. */
+ @Test
+ public void testMissingDataRegion(@InjectConfiguration DataStorageConfiguration dbCfg) throws Exception {
+ tableCfg.tables().get("table").dataRegion().update("r0").get(1, TimeUnit.SECONDS);
+
+ ValidationContext<NamedListView<TableView>> ctx = mockContext(null, tableCfg.tables().value(), dbCfg.value());
+
+ ArgumentCaptor<ValidationIssue> issuesCaptor = ArgumentCaptor.forClass(ValidationIssue.class);
+
+ doNothing().when(ctx).addIssue(issuesCaptor.capture());
+
+ TableValidatorImpl.INSTANCE.validate(null, ctx);
+
+ assertEquals(1, issuesCaptor.getAllValues().size());
+
+ assertEquals(
+ "Data region 'r0' configured for table 'schema.table' isn't found",
+ issuesCaptor.getValue().message()
+ );
+ }
+
+ /** Tests that new data region must have the same type. */
+ @Test
+ public void testChangeDataRegionType(
+ @InjectConfiguration("mock.regions.r0.type = foo") DataStorageConfiguration dbCfg
+ ) throws Exception {
+ NamedListView<TableView> oldValue = tableCfg.tables().value();
+
+ tableCfg.tables().get("table").dataRegion().update("r0").get(1, TimeUnit.SECONDS);
+
+ NamedListView<TableView> newValue = tableCfg.tables().value();
+
+ ValidationContext<NamedListView<TableView>> ctx = mockContext(oldValue, newValue, dbCfg.value());
+
+ ArgumentCaptor<ValidationIssue> issuesCaptor = ArgumentCaptor.forClass(ValidationIssue.class);
+
+ doNothing().when(ctx).addIssue(issuesCaptor.capture());
+
+ TableValidatorImpl.INSTANCE.validate(null, ctx);
+
+ assertEquals(1, issuesCaptor.getAllValues().size());
+
+ assertEquals(
+ "Unable to move table 'schema.table' from region 'default' to region 'r0' because it has different type (foo)",
+ issuesCaptor.getValue().message()
+ );
+ }
+
+ /**
+ * Mocks validation context.
+ *
+ * @param oldValue Old value of configuration.
+ * @param newValue New value of configuration.
+ * @param dbCfgView Data storage configuration to register it by {@link DataStorageConfiguration#KEY}.
+ * @return Mocked validation context.
+ */
+ private static ValidationContext<NamedListView<TableView>> mockContext(
+ NamedListView<TableView> oldValue,
Review comment:
```suggestion
@Nullable NamedListView<TableView> oldValue,
```
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -57,10 +64,68 @@
}
catch (IllegalArgumentException e) {
ctx.addIssue(new ValidationIssue("Validator works success by key " + ctx.currentKey() + ". Found "
- + view.columns().size() + " columns"));
+ + newTable.columns().size() + " columns"));
}
+
+ validateDataRegion(oldTables == null ? null : oldTables.get(tableName), newTable, ctx);
}
+ }
+
+ /**
+ * Validate data region validity.
+ *
+ * @param oldTable Previous configuration, maybe {@code null}.
+ * @param newTable New configuration.
+ * @param ctx Validation context.
+ */
+ // TODO Rename isn't supported right now.
+ private void validateDataRegion(TableView oldTable, TableView newTable, ValidationContext<?> ctx) {
+ DataStorageView oldDbCfg = ctx.getOldRoot(DataStorageConfiguration.KEY);
+ DataStorageView newDbCfg = ctx.getNewRoot(DataStorageConfiguration.KEY);
+
+ if (oldTable != null && Objects.equals(oldTable.dataRegion(), newTable.dataRegion()))
+ return;
+
+ DataRegionView newRegion = dataRegion(newDbCfg, newTable.dataRegion());
+
+ if (newRegion == null) {
+ ctx.addIssue(new ValidationIssue(String.format(
+ "Data region '%s' configured for table '%s' isn't found",
+ newTable.dataRegion(),
+ newTable.name()
+ )));
+
+ return;
+ }
+
+ if (oldDbCfg == null || oldTable == null)
+ return;
+
+ DataRegionView oldRegion = dataRegion(oldDbCfg, oldTable.dataRegion());
+
+ if (!oldRegion.type().equalsIgnoreCase(newRegion.type())) {
+ ctx.addIssue(new ValidationIssue(String.format(
+ "Unable to move table '%s' from region '%s' to region '%s' because it has different type (%s)",
Review comment:
I would specify both the existing type and the new type
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/TableValidatorImpl.java
##########
@@ -57,10 +64,68 @@
}
catch (IllegalArgumentException e) {
ctx.addIssue(new ValidationIssue("Validator works success by key " + ctx.currentKey() + ". Found "
- + view.columns().size() + " columns"));
+ + newTable.columns().size() + " columns"));
}
+
+ validateDataRegion(oldTables == null ? null : oldTables.get(tableName), newTable, ctx);
}
+ }
+
+ /**
+ * Validate data region validity.
+ *
+ * @param oldTable Previous configuration, maybe {@code null}.
+ * @param newTable New configuration.
+ * @param ctx Validation context.
+ */
+ // TODO Rename isn't supported right now.
+ private void validateDataRegion(TableView oldTable, TableView newTable, ValidationContext<?> ctx) {
+ DataStorageView oldDbCfg = ctx.getOldRoot(DataStorageConfiguration.KEY);
+ DataStorageView newDbCfg = ctx.getNewRoot(DataStorageConfiguration.KEY);
+
+ if (oldTable != null && Objects.equals(oldTable.dataRegion(), newTable.dataRegion()))
+ return;
+
+ DataRegionView newRegion = dataRegion(newDbCfg, newTable.dataRegion());
+
+ if (newRegion == null) {
+ ctx.addIssue(new ValidationIssue(String.format(
+ "Data region '%s' configured for table '%s' isn't found",
+ newTable.dataRegion(),
+ newTable.name()
+ )));
+
+ return;
+ }
+
+ if (oldDbCfg == null || oldTable == null)
+ return;
+
+ DataRegionView oldRegion = dataRegion(oldDbCfg, oldTable.dataRegion());
+
+ if (!oldRegion.type().equalsIgnoreCase(newRegion.type())) {
+ ctx.addIssue(new ValidationIssue(String.format(
+ "Unable to move table '%s' from region '%s' to region '%s' because it has different type (%s)",
+ newTable.name(),
+ oldTable.dataRegion(),
+ newTable.dataRegion(),
+ newRegion.type()
+ )));
+ }
+ }
+
+ /**
+ * Retrieves data region configuration.
+ *
+ * @param dbCfg Data storage configuration.
+ * @param regionName Data region name.
+ * @return Data region configuration.
+ */
+ private DataRegionView dataRegion(DataStorageView dbCfg, String regionName) {
Review comment:
```suggestion
private static DataRegionView dataRegion(DataStorageView dbCfg, String regionName) {
```
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/TableValidatorImplTest.java
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.schema.configuration;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.configuration.schemas.store.DataStorageView;
+import org.apache.ignite.configuration.schemas.table.TableView;
+import org.apache.ignite.configuration.schemas.table.TablesConfiguration;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/** */
+@ExtendWith(ConfigurationExtension.class)
+public class TableValidatorImplTest {
+ /** Basic table configuration to mutate and then validate. */
+ @InjectConfiguration("mock.tables.table {\n" +
+ " name = schema.table,\n" +
+ " columns.0 {name = id, type.type = STRING, nullable = true},\n" +
+ " primaryKey {columns = [id], affinityColumns = [id]}\n" +
+ "}")
+ private TablesConfiguration tableCfg;
+
+ /** Tests that validater catches nonexistent data regions. */
Review comment:
```suggestion
/** Tests that the validator catches nonexistent data regions. */
```
--
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