korlov42 commented on code in PR #3075: URL: https://github.com/apache/ignite-3/pull/3075#discussion_r1464925345
########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/SnapshotUpdate.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.storage; + +import java.util.List; +import org.apache.ignite.internal.tostring.S; + +/** + * Group of changes that relates to specified version. + */ +public class SnapshotUpdate extends VersionedUpdate { Review Comment: the fact that `SnapshotUpdate` extends `VersionedUpdate` doesn't feel right, because `VersionedUpdate` has semantic of "number of changes that brings the catalog of version `versionUpdate.version - 1` to the desired state of version `versionUpdate.version`. On the other hand, `SnapshotUpdate` simply persists cumulative state of catalog of particular version. I think, it would be better to keep these classes apart in order to avoid confusion. Also, if we will follow this way, we won't be need `SnapshotEntry` (which is also quite confusing) ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -418,6 +440,15 @@ public List<SystemView<?>> systemViews() { class OnUpdateHandlerImpl implements OnUpdateHandler { @Override public CompletableFuture<Void> handle(VersionedUpdate update, HybridTimestamp metaStorageUpdateTimestamp, long causalityToken) { + if (update instanceof SnapshotUpdate) { + Catalog catalog = ((SnapshotUpdate) update).snapshotEntry().applyUpdate(catalogByVer.get(0), causalityToken); Review Comment: after the first compaction the catalog of version 0 will be truncated as well. This is kinda confusing (I mean, we definitely will get null here, why do we even do this in the first place?) ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/SnapshotUpdate.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.storage; + +import java.util.List; +import org.apache.ignite.internal.tostring.S; + +/** + * Group of changes that relates to specified version. Review Comment: copy-pasted javadoc ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -330,11 +333,30 @@ public CompletableFuture<Void> execute(List<CatalogCommand> commands) { return saveUpdateAndWaitForActivation(new BulkUpdateProducer(List.copyOf(commands))); } + /** + * Cleanup outdated catalog versions, which can't be observed after given timestamp (inclusively), and compact underlying update log. + * + * @param timestamp Earliest observable timestamp. + * @return Operation future. + */ + public CompletableFuture<Void> compactCatalog(long timestamp) { + Catalog catalog = catalogAt(timestamp); + + truncateUpTo(catalog.version(), catalog.time()); Review Comment: javadoc of `UpdateLog#saveSnapshot` states that snapshotting might not be supported. In that case we will truncate history on only current node, while others will be left intact. Does it make sense to clean the history in only one place -- OnUpdateHandler? I believe this will allow us to preserve consistent behaviour across all the nodes of the cluster ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -418,6 +440,15 @@ public List<SystemView<?>> systemViews() { class OnUpdateHandlerImpl implements OnUpdateHandler { @Override public CompletableFuture<Void> handle(VersionedUpdate update, HybridTimestamp metaStorageUpdateTimestamp, long causalityToken) { + if (update instanceof SnapshotUpdate) { + Catalog catalog = ((SnapshotUpdate) update).snapshotEntry().applyUpdate(catalogByVer.get(0), causalityToken); + + registerCatalog(catalog); Review Comment: when I first looked at this it was unclear why do we need to register version once again... Turns out we need to register snapshot during the recovery phase. I believe it worth to mention in comment to this particular row ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateLog.java: ########## @@ -41,6 +41,15 @@ public interface UpdateLog extends IgniteComponent { */ CompletableFuture<Boolean> append(VersionedUpdate update); + /** + * Saves an snapshot entry and drop updates of previous versions from the log, if supported, otherwise do nothing. + * + * @param snapshotEntry An entry, which represents a result of merging updates of previous versions. + * @return A {@code true} if snapshot has been successfully appended, {@code false} otherwise + * if update with the same version already exists, or snapshots are not unsupported. + */ Review Comment: `an snapshot` --> `a snapshot` `if update with the same version already exists` -- well, as far as I understand, update with the same version _must_ exists (effectively, you are replacing head of the update log N->M with snapshot of version M) `are not unsupported.` --> did you mean `are not supported.`? ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/UpdateLogImpl.java: ########## @@ -198,6 +251,10 @@ static ByteArray update(int version) { static ByteArray updatePrefix() { return ByteArray.fromString("catalog.update."); } + + public static ByteArray snapshotVersion() { + return ByteArray.fromString("catalog.snapshot.version"); Review Comment: @zstan , elaborate please, what is wrong with current key? It seems in line with rest of the catalog's keys ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -330,11 +333,30 @@ public CompletableFuture<Void> execute(List<CatalogCommand> commands) { return saveUpdateAndWaitForActivation(new BulkUpdateProducer(List.copyOf(commands))); } + /** + * Cleanup outdated catalog versions, which can't be observed after given timestamp (inclusively), and compact underlying update log. + * + * @param timestamp Earliest observable timestamp. + * @return Operation future. + */ + public CompletableFuture<Void> compactCatalog(long timestamp) { + Catalog catalog = catalogAt(timestamp); + + truncateUpTo(catalog.version(), catalog.time()); + + return updateLog.saveSnapshot(new SnapshotUpdate(catalog.version(), new SnapshotEntry(catalog))).thenAccept(ignore -> {}); + } + private void registerCatalog(Catalog newCatalog) { catalogByVer.put(newCatalog.version(), newCatalog); catalogByTs.put(newCatalog.time(), newCatalog); } + private void truncateUpTo(int version, long time) { Review Comment: If we will accept an entire `Catalog`, then there will be no chance to provide inconsistent `version` and `time`. 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
