rpuch commented on code in PR #3561: URL: https://github.com/apache/ignite-3/pull/3561#discussion_r1565299243
########## modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/TestStorageUtils.java: ########## @@ -0,0 +1,34 @@ +/* + * 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.storage; + +import org.apache.ignite.internal.storage.index.IndexStorage; + +/** Auxiliary class for tests that contains useful methods, constants, etc. */ +public class TestStorageUtils { + /** Completes the building of indexes. */ + public static void completeBuiltIndexes(MvPartitionStorage partitionStorage, IndexStorage... indexStorages) { Review Comment: It's not clear what this method does from its name and its javadoc and whether this is some shortcut for tests (as it looks) and not something useful for production code. Can a better name be devised? Like `makeIndexStoragesLookBuilt`? Or `forceIndexStoragesBuildCompletion`? ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexStorage.java: ########## @@ -56,13 +57,20 @@ public interface SortedIndexStorage extends IndexStorage { * {@code null} means unbounded. * @param flags Control flags. {@link #GREATER} | {@link #LESS} by default. Other available values * are {@link #GREATER_OR_EQUAL}, {@link #LESS_OR_EQUAL}. + * @param onlyBuiltIndex {@code True} if need to scan only from the built index. * @return Cursor with fetched index rows. * @throws IllegalArgumentException If backwards flag is passed and backwards iteration is not supported by the storage. + * @throws StorageException If failed to read data. + * @throws IndexNotBuiltException If the index has not yet been built and {@code onlyBuiltIndex} = {@code true}. Review Comment: Otherwise there might be a confusion whether `=` means an assignment or an equality check ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexStorage.java: ########## @@ -56,13 +57,20 @@ public interface SortedIndexStorage extends IndexStorage { * {@code null} means unbounded. * @param flags Control flags. {@link #GREATER} | {@link #LESS} by default. Other available values * are {@link #GREATER_OR_EQUAL}, {@link #LESS_OR_EQUAL}. + * @param onlyBuiltIndex {@code True} if need to scan only from the built index. * @return Cursor with fetched index rows. * @throws IllegalArgumentException If backwards flag is passed and backwards iteration is not supported by the storage. + * @throws StorageException If failed to read data. + * @throws IndexNotBuiltException If the index has not yet been built and {@code onlyBuiltIndex} = {@code true}. + * @throws InconsistentIndexStateException If the index is in a readable status, but the index is not built. */ + // TODO: IGNITE-22039 Implement for onlyBuiltIndex == false throw an InconsistentIndexStateException if the index is not in a readable + // status and write tests PeekCursor<IndexRow> scan( @Nullable BinaryTuplePrefix lowerBound, @Nullable BinaryTuplePrefix upperBound, - @MagicConstant(flagsFromClass = SortedIndexStorage.class) int flags + @MagicConstant(flagsFromClass = SortedIndexStorage.class) int flags, + boolean onlyBuiltIndex Review Comment: The usage of `true` and `false` is very unequal in the project. `false` is only used once, and it's in a very special case, while `true` is used 'normally'. How about making 2 methods: one would be a normal `scan()` with the same parameters that it had before this change (and its implementation will imply `onlyBuiltIndex == true`); and another like `tolerantScan()` that would tolerate inconsistencies (again, without the boolean parameter, instead it would just imply `onlyBuiltIndex == false`). This would allow us to avoid polluting the code with that `true` everywhere we want to just scan the index without bringing any confusion, and the special case would be handled by the special method. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexStorage.java: ########## @@ -56,13 +57,20 @@ public interface SortedIndexStorage extends IndexStorage { * {@code null} means unbounded. * @param flags Control flags. {@link #GREATER} | {@link #LESS} by default. Other available values * are {@link #GREATER_OR_EQUAL}, {@link #LESS_OR_EQUAL}. + * @param onlyBuiltIndex {@code True} if need to scan only from the built index. * @return Cursor with fetched index rows. * @throws IllegalArgumentException If backwards flag is passed and backwards iteration is not supported by the storage. + * @throws StorageException If failed to read data. + * @throws IndexNotBuiltException If the index has not yet been built and {@code onlyBuiltIndex} = {@code true}. + * @throws InconsistentIndexStateException If the index is in a readable status, but the index is not built. */ + // TODO: IGNITE-22039 Implement for onlyBuiltIndex == false throw an InconsistentIndexStateException if the index is not in a readable + // status and write tests PeekCursor<IndexRow> scan( @Nullable BinaryTuplePrefix lowerBound, @Nullable BinaryTuplePrefix upperBound, - @MagicConstant(flagsFromClass = SortedIndexStorage.class) int flags + @MagicConstant(flagsFromClass = SortedIndexStorage.class) int flags, + boolean onlyBuiltIndex Review Comment: It's not clear that `onlyBuiltIndex` means 'throw if it's not built'. Maybe just name it `throwIfNotBuilt`? ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/index/SortedIndexStorage.java: ########## @@ -56,13 +57,20 @@ public interface SortedIndexStorage extends IndexStorage { * {@code null} means unbounded. * @param flags Control flags. {@link #GREATER} | {@link #LESS} by default. Other available values * are {@link #GREATER_OR_EQUAL}, {@link #LESS_OR_EQUAL}. + * @param onlyBuiltIndex {@code True} if need to scan only from the built index. * @return Cursor with fetched index rows. * @throws IllegalArgumentException If backwards flag is passed and backwards iteration is not supported by the storage. + * @throws StorageException If failed to read data. + * @throws IndexNotBuiltException If the index has not yet been built and {@code onlyBuiltIndex} = {@code true}. Review Comment: ```suggestion * @throws IndexNotBuiltException If the index has not yet been built and {@code onlyBuiltIndex} is {@code true}. ``` -- 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]
