You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ibessonov (via GitHub)" <gi...@apache.org> on 2024/01/05 14:14:12 UTC
[PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
ibessonov opened a new pull request, #3013:
URL: https://github.com/apache/ignite-3/pull/3013
https://issues.apache.org/jira/browse/IGNITE-21205
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1444997446
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -208,15 +218,19 @@ private void setColumnMapping(SchemaDescriptor schema, int tableId) throws Execu
* @return Schema representation.
*/
private SchemaDescriptor loadSchemaDescriptor(int tblId, int ver) {
- Entry entry = metastorageMgr.getLocally(schemaWithVerHistKey(tblId, ver), Long.MAX_VALUE);
+ int version = catalogService.latestCatalogVersion();
- assert !entry.tombstone() : "Table " + tblId + ", version " + ver;
+ while (version >= catalogService.earliestCatalogVersion()) {
+ CatalogTableDescriptor tableDescriptor = catalogService.table(tblId, version);
- byte[] value = entry.value();
+ if (tableDescriptor == null) {
+ continue;
Review Comment:
looks like `version` is not updated, causing this loop to never end... It would be nice to cover this case with test
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -107,10 +94,34 @@ private void registerExistingTables() {
// exactly matching the tables).
Review Comment:
is IGNITE-20051 still valid after this patch?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -47,6 +48,8 @@ public class CatalogTableDescriptor extends CatalogObjectDescriptor {
private final int tableVersion;
Review Comment:
probably, it would be better to remove `tableVersion` at all, rename `CatalogTableDescriptor#tableVersion` to `latestVersion` and delegate call to `schemaVersions`, 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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1446945996
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -103,6 +133,22 @@ public CatalogTableDescriptor(
assert Set.copyOf(primaryKeyColumns).containsAll(colocationColumns);
}
+ /**
+ * Creates new table descriptor, using existing one as a template.
+ */
+ public CatalogTableDescriptor newDescriptor(
+ String name,
+ int tableVersion,
Review Comment:
Version may not change when you create a new descriptor
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445859277
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
+ }
+ }
+
+ private final int base;
+ private final TableVersion[] versions;
+
+ /**
+ * Constructor.
+ *
+ * @param versions Array of table versions.
+ */
+ CatalogTableSchemaVersions(TableVersion... versions) {
+ this(CatalogTableDescriptor.INITIAL_TABLE_VERSION, versions);
+ }
+
+ private CatalogTableSchemaVersions(int base, TableVersion... versions) {
+ this.base = base;
+ this.versions = versions;
+ }
+
+ /**
+ * Returns earliest known table version.
+ */
+ public int earliestVersion() {
+ return base;
+ }
+
+ /**
+ * Returns latest known table version.
+ */
+ public int latestVersion() {
+ return base + versions.length - 1;
+ }
+
+ /**
+ * Returns an existing table version, or {@code null} if it's not found.
+ */
+ public @Nullable TableVersion get(int version) {
+ try {
+ return versions[version - base];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
I expect that it would never happen, but ok, I'll change the code
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov merged PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1444305614
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -227,19 +241,7 @@ private SchemaDescriptor loadSchemaDescriptor(int tblId, int ver) {
* @return Future that will be completed when the schema gets saved.
*/
private CompletableFuture<Void> saveSchemaDescriptor(int tableId, SchemaDescriptor schema) {
- ByteArray schemaKey = schemaWithVerHistKey(tableId, schema.version());
- ByteArray latestSchemaVersionKey = latestSchemaVersionKey(tableId);
-
- byte[] serializedSchema = SchemaSerializerImpl.INSTANCE.serialize(schema);
-
- return metastorageMgr.invoke(
- notExists(schemaKey),
- List.of(
- put(schemaKey, serializedSchema),
- put(latestSchemaVersionKey, intToBytes(schema.version()))
- ),
- emptyList()
- ).thenApply(unused -> null);
+ return nullCompletedFuture();
Review Comment:
Comment to myself: I forgot to remove this method.
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1444339966
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
+ }
+ }
+
+ private final int base;
+ private final TableVersion[] versions;
+
+ /**
+ * Constructor.
+ *
+ * @param versions Array of table versions.
+ */
+ CatalogTableSchemaVersions(TableVersion... versions) {
+ this(CatalogTableDescriptor.INITIAL_TABLE_VERSION, versions);
+ }
+
+ private CatalogTableSchemaVersions(int base, TableVersion... versions) {
+ this.base = base;
+ this.versions = versions;
+ }
+
+ /**
+ * Returns earliest known table version.
+ */
+ public int earliestVersion() {
+ return base;
+ }
+
+ /**
+ * Returns latest known table version.
+ */
+ public int latestVersion() {
+ return base + versions.length - 1;
+ }
+
+ /**
+ * Returns an existing table version, or {@code null} if it's not found.
+ */
+ public @Nullable TableVersion get(int version) {
+ try {
+ return versions[version - base];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ return null;
+ }
+ }
+
+ /**
+ * Creates a new instance of {@link CatalogTableSchemaVersions} with one new version appened.
Review Comment:
```suggestion
* Creates a new instance of {@link CatalogTableSchemaVersions} with one new version appended.
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
Review Comment:
It seems that `TableVersion` holds all the state of a table description that might be changed during the lifetime of a table. If this is the case, shouldn't we also include `name` here (as a table might be renamed)? Or it's left out for now, while table renamings are not implemented?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
+ }
+ }
+
+ private final int base;
+ private final TableVersion[] versions;
+
+ /**
+ * Constructor.
+ *
+ * @param versions Array of table versions.
+ */
+ CatalogTableSchemaVersions(TableVersion... versions) {
+ this(CatalogTableDescriptor.INITIAL_TABLE_VERSION, versions);
+ }
+
+ private CatalogTableSchemaVersions(int base, TableVersion... versions) {
+ this.base = base;
+ this.versions = versions;
+ }
+
+ /**
+ * Returns earliest known table version.
+ */
+ public int earliestVersion() {
+ return base;
+ }
+
+ /**
+ * Returns latest known table version.
+ */
+ public int latestVersion() {
+ return base + versions.length - 1;
+ }
+
+ /**
+ * Returns an existing table version, or {@code null} if it's not found.
+ */
+ public @Nullable TableVersion get(int version) {
+ try {
+ return versions[version - base];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
It seems that explicitly checking for version range will be cheaper than creating an exception
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -107,10 +94,34 @@ private void registerExistingTables() {
// exactly matching the tables).
long causalityToken = metastorageMgr.appliedRevision();
- int catalogVersion = catalogService.latestCatalogVersion();
- for (CatalogTableDescriptor tableDescriptor : catalogService.tables(catalogVersion)) {
- onTableCreated(new CreateTableEventParameters(causalityToken, catalogVersion, tableDescriptor), null);
+ for (int v = catalogService.latestCatalogVersion(); v >= catalogService.earliestCatalogVersion(); v--) {
+ Collection<CatalogTableDescriptor> tables = catalogService.tables(v);
+
+ registriesVv.update(causalityToken, (registries, throwable) -> {
+ for (CatalogTableDescriptor tableDescriptor : tables) {
+ int tableId = tableDescriptor.id();
+
+ if (registries.containsKey(tableId)) {
+ continue;
+ }
+
+ SchemaDescriptor prevSchema = null;
+ CatalogTableSchemaVersions schemaVersions = tableDescriptor.schemaVersions();
+ for (int ver = schemaVersions.earliestVersion(); ver <= schemaVersions.latestVersion(); ver++) {
Review Comment:
`v` is a CatalogVersion, but `ver` is a `TableVersion`, so basically different domains. I think it makes sense to rename the variables (like `catVersion`, `tableVersion`) to avoid confusion.
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/catalog/CatalogToSchemaDescriptorConverter.java:
##########
@@ -149,15 +149,15 @@ public static Column convert(int columnOrder, CatalogTableColumnDescriptor colum
* @param tableDescriptor Descriptor to convert.
* @return A {@link SchemaDescriptor} object representing the table descriptor.
*/
- public static SchemaDescriptor convert(CatalogTableDescriptor tableDescriptor) {
+ public static SchemaDescriptor convert(CatalogTableDescriptor tableDescriptor, int tableVersion) {
Set<String> keyColumnsNames = Set.copyOf(tableDescriptor.primaryKeyColumns());
List<Column> keyCols = new ArrayList<>(keyColumnsNames.size());
List<Column> valCols = new ArrayList<>(tableDescriptor.columns().size() - keyColumnsNames.size());
int idx = 0;
- for (CatalogTableColumnDescriptor column : tableDescriptor.columns()) {
+ for (CatalogTableColumnDescriptor column : tableDescriptor.schemaVersions().get(tableVersion).columns()) {
Review Comment:
`get()` might return `null`. Let's explicitly assert that it's not `null` if we are sure this should never happen.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
Review Comment:
How about doing a defensive copy in the constructor like `this columns = List.copyOf(columns)` one time instead of creating an unmodifiable wrapper on each access?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -103,6 +133,22 @@ public CatalogTableDescriptor(
assert Set.copyOf(primaryKeyColumns).containsAll(colocationColumns);
}
+ /**
+ * Creates new table descriptor, using existing one as a template.
+ */
+ public CatalogTableDescriptor newDescriptor(
+ String name,
+ int tableVersion,
Review Comment:
Why do we need to pass this explicitly? Can't we just calculate it inside?
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -208,15 +218,19 @@ private void setColumnMapping(SchemaDescriptor schema, int tableId) throws Execu
* @return Schema representation.
*/
private SchemaDescriptor loadSchemaDescriptor(int tblId, int ver) {
- Entry entry = metastorageMgr.getLocally(schemaWithVerHistKey(tblId, ver), Long.MAX_VALUE);
+ int version = catalogService.latestCatalogVersion();
Review Comment:
This is a Catalog version, while `ver` is a table version. Let's rename them accordingly
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445865204
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
Review Comment:
I don't want to create a copy of the list, having RO wrapper is cheaper. I would go as far as returning the mutable instance, but you wouldn't accept that
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445866302
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -103,6 +133,22 @@ public CatalogTableDescriptor(
assert Set.copyOf(primaryKeyColumns).containsAll(colocationColumns);
}
+ /**
+ * Creates new table descriptor, using existing one as a template.
+ */
+ public CatalogTableDescriptor newDescriptor(
+ String name,
+ int tableVersion,
Review Comment:
You mean comparing latest known columns with the list that's in parameters? That's expensive. If the information is already known outside, we should use 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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445876223
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -208,15 +218,19 @@ private void setColumnMapping(SchemaDescriptor schema, int tableId) throws Execu
* @return Schema representation.
*/
private SchemaDescriptor loadSchemaDescriptor(int tblId, int ver) {
- Entry entry = metastorageMgr.getLocally(schemaWithVerHistKey(tblId, ver), Long.MAX_VALUE);
+ int version = catalogService.latestCatalogVersion();
- assert !entry.tombstone() : "Table " + tblId + ", version " + ver;
+ while (version >= catalogService.earliestCatalogVersion()) {
+ CatalogTableDescriptor tableDescriptor = catalogService.table(tblId, version);
- byte[] value = entry.value();
+ if (tableDescriptor == null) {
+ continue;
Review Comment:
I believe that writing a test would be problematic right now, this might be a dead code that's never executed actually, let's investigate it separately
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445867976
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/catalog/CatalogToSchemaDescriptorConverter.java:
##########
@@ -149,15 +149,15 @@ public static Column convert(int columnOrder, CatalogTableColumnDescriptor colum
* @param tableDescriptor Descriptor to convert.
* @return A {@link SchemaDescriptor} object representing the table descriptor.
*/
- public static SchemaDescriptor convert(CatalogTableDescriptor tableDescriptor) {
+ public static SchemaDescriptor convert(CatalogTableDescriptor tableDescriptor, int tableVersion) {
Set<String> keyColumnsNames = Set.copyOf(tableDescriptor.primaryKeyColumns());
List<Column> keyCols = new ArrayList<>(keyColumnsNames.size());
List<Column> valCols = new ArrayList<>(tableDescriptor.columns().size() - keyColumnsNames.size());
int idx = 0;
- for (CatalogTableColumnDescriptor column : tableDescriptor.columns()) {
+ for (CatalogTableColumnDescriptor column : tableDescriptor.schemaVersions().get(tableVersion).columns()) {
Review Comment:
Ok
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445878645
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -47,6 +48,8 @@ public class CatalogTableDescriptor extends CatalogObjectDescriptor {
private final int tableVersion;
Review Comment:
I think it's a good idea
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445863547
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
Review Comment:
It's not about name and changes, it's only about sets of columns
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445866605
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -107,10 +94,34 @@ private void registerExistingTables() {
// exactly matching the tables).
long causalityToken = metastorageMgr.appliedRevision();
- int catalogVersion = catalogService.latestCatalogVersion();
- for (CatalogTableDescriptor tableDescriptor : catalogService.tables(catalogVersion)) {
- onTableCreated(new CreateTableEventParameters(causalityToken, catalogVersion, tableDescriptor), null);
+ for (int v = catalogService.latestCatalogVersion(); v >= catalogService.earliestCatalogVersion(); v--) {
+ Collection<CatalogTableDescriptor> tables = catalogService.tables(v);
+
+ registriesVv.update(causalityToken, (registries, throwable) -> {
+ for (CatalogTableDescriptor tableDescriptor : tables) {
+ int tableId = tableDescriptor.id();
+
+ if (registries.containsKey(tableId)) {
+ continue;
+ }
+
+ SchemaDescriptor prevSchema = null;
+ CatalogTableSchemaVersions schemaVersions = tableDescriptor.schemaVersions();
+ for (int ver = schemaVersions.earliestVersion(); ver <= schemaVersions.latestVersion(); ver++) {
Review Comment:
Ok
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445872146
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -208,15 +218,19 @@ private void setColumnMapping(SchemaDescriptor schema, int tableId) throws Execu
* @return Schema representation.
*/
private SchemaDescriptor loadSchemaDescriptor(int tblId, int ver) {
- Entry entry = metastorageMgr.getLocally(schemaWithVerHistKey(tblId, ver), Long.MAX_VALUE);
+ int version = catalogService.latestCatalogVersion();
- assert !entry.tombstone() : "Table " + tblId + ", version " + ver;
+ while (version >= catalogService.earliestCatalogVersion()) {
+ CatalogTableDescriptor tableDescriptor = catalogService.table(tblId, version);
- byte[] value = entry.value();
+ if (tableDescriptor == null) {
+ continue;
Review Comment:
Good catch, thank you!
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445870629
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java:
##########
@@ -107,10 +94,34 @@ private void registerExistingTables() {
// exactly matching the tables).
Review Comment:
No
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445926315
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableSchemaVersions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.catalog.descriptors;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+import org.apache.ignite.internal.util.ArrayUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class that holds a list of table version descriptors.
+ */
+public class CatalogTableSchemaVersions implements Serializable {
+ private static final long serialVersionUID = 4353352473287209850L;
+
+ /**
+ * Descriptor of a single table version.
+ */
+ public static class TableVersion implements Serializable {
+ private static final long serialVersionUID = -5185852983239967322L;
+
+ private final List<CatalogTableColumnDescriptor> columns;
+
+ TableVersion(List<CatalogTableColumnDescriptor> columns) {
+ this.columns = columns;
+ }
+
+ public List<CatalogTableColumnDescriptor> columns() {
+ return Collections.unmodifiableList(columns);
Review Comment:
Looks like `columns` is always a new instance, so no sharing should happen, everything is fine BTW, we could use `toUnmodifiableList()` on the calling side, in which case even `copyOf()` would become basically a no-op.
--
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
Re: [PR] IGNITE-21205 Get rid of metastorage in schema manager [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #3013:
URL: https://github.com/apache/ignite-3/pull/3013#discussion_r1445926863
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -103,6 +133,22 @@ public CatalogTableDescriptor(
assert Set.copyOf(primaryKeyColumns).containsAll(colocationColumns);
}
+ /**
+ * Creates new table descriptor, using existing one as a template.
+ */
+ public CatalogTableDescriptor newDescriptor(
+ String name,
+ int tableVersion,
Review Comment:
I mean using `tableVersion + 1` to calculate the next table version :)
--
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