IGNITE-20620 Add index availability command to catalog #2680

tkalkirill · 2023-10-11T14:43:01Z

https://issues.apache.org/jira/browse/IGNITE-20620

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

...c/main/java/org/apache/ignite/internal/catalog/IndexAlreadyAvailableValidationException.java

...talog/src/main/java/org/apache/ignite/internal/catalog/IndexNotFoundValidationException.java

rpuch · 2023-10-12T06:53:35Z

...talog/src/main/java/org/apache/ignite/internal/catalog/IndexNotFoundValidationException.java

 *
- * <p>This exception is used to properly handle IF EXISTS flag in ddl command handler.
+ * <p>Example: This exception is used to properly handle IF EXISTS flag in ddl command handler.</p>


Do we really need that trailing </p> ?

Why not? =)

In the code that I recall we never use it. But this is not too important

rpuch · 2023-10-12T06:55:26Z

modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java

+     * @return Table with given name.
+     * @throws IndexNotFoundValidationException If index with given name is not exists.
+     */
+    static CatalogIndexDescriptor indexOrThrow(CatalogSchemaDescriptor schema, String name) throws IndexNotFoundValidationException {


How about moving this method to CatalogSchemaDescriptor?

I don’t think it’s worth it, this method is only used by teams, but CatalogSchemaDescriptor can be used by anyone.

rpuch · 2023-10-12T06:56:59Z

modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java

+        CatalogIndexDescriptor index = schema.index(name);
+
+        if (index == null) {
+            throw new IndexNotFoundValidationException(format("Index with name '{}.{}' not found", schema.name(), name));


It looks like this method can be used not only in the context where we validate something, but it throws a validation exception. Can the exception type be generalized? Or the method renamed (or at least documented) that it's just for validation phase?

In this class there were methods schemaOrThrow and tableOrThrow, I don’t think that at the moment there is a need to spoil the consistency in the naming, plus the semantics and documentation of the method indicate that it will be a validation exception that will be thrown.

If the whole CatalogUtils is about validation stage, the class itself can be renamed. Or these methods might be extracted to a class that's about validation stage logic.

I think it's worth fixing this outside of the current task.

rpuch · 2023-10-12T07:51:57Z

...atalog/src/main/java/org/apache/ignite/internal/catalog/storage/MakeIndexAvailableEntry.java

+    }
+
+    @Override
+    public Catalog applyUpdate(Catalog catalog, long causalityToken) {


This method implementation is not specific to making an index available: it just replaces the current descriptor with a new one. Would it make sense to extract it to a static method (or use subclassing) in the light of more 'index modification' commands that are going to be added next?

I don’t know if this should be put into a separate method; if in the future we need to reuse the code, then we’ll redo it, but for now we’ll leave it as it is.

...ava/org/apache/ignite/internal/catalog/commands/MakeIndexAvailableCommandValidationTest.java

...log/src/main/java/org/apache/ignite/internal/catalog/commands/MakeIndexAvailableCommand.java

.../main/java/org/apache/ignite/internal/catalog/commands/MakeIndexAvailableCommandBuilder.java

rpuch

It seems that we still have both 'available' and 'WO/RO' terminologies both present. I would be great to only use one of them. For instance, in the CatalogIndexDescriptor, it's writeOnly, but the command is about making available.

...c/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogSortedIndexDescriptor.java

* IGNITE-19425 Use data nodes from DistributionZoneManager instead of BaselineManager#nodes and remove BaselineManager (apache#2605) * IGNITE-20127 Implement delayed replication acks - Fixes apache#2520. Signed-off-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> * IGNITE-20330 Create an abstraction for building indexes (apache#2631) * IGNITE-20513 Use NativeType as the type of the system view column (apache#2668) * IGNITE-20590 Remove TxManager.beginImplicit() (apache#2669) * IGNITE-20588 Use consistent schema when converting data in KV/record views (apache#2667) * IGNITE-20342 Rollback transaction for SQL execution issues (apache#2611) * IGNITE-20445 Clean up write intents for RW transaction on replication… (apache#2657) * IGNITE-20607 Move NativeType to core (apache#2670) * IGNITE-20484 NPE when some operation occurs when the primary replica … (apache#2672) * IGNITE-20442 Sql. Extend grammar with transaction related statements. (apache#2663) * IGNITE-20620 Add index availability command to catalog (apache#2680) * IGNITE-20521 Split Security API from ignite-security module (apache#2662) * IGNITE-20621 Add a replication group index build completion listener to IndexBuilder (apache#2682) * IGNITE-19226 Fetch table schema by timestamp (apache#2648) * IGNITE-19325 Unmute test MultiActorPlacementDriverTest#prolongAfterActiveActorChanger (apache#2677) * IGNITE-20625 Fix ODBC metadata reading error (apache#2687) * IGNITE-20457 Verify commitTimestamp against enlisted partitions expiration timestamps (apache#2658) * IGNITE-20635 Cleanup code wrt IGNITE-18733 mentions (apache#2686) * IGNITE-20317 Return metastorage invokes for zones changes in handlers, immediately recalculate data nodes when scale up/down is immediate. (apache#2685) * IGNITE-19276 Implement a mechanism to build indices distributively (apache#2676) * IGNITE-20567 Move the 'enabled' flag from the authentication configuration to security (apache#2665) * IGNITE-20578 Implement scan over system view (apache#2678) * IGNITE-20387: Remap most exceptions to SqlExceptions for SQL API (apache#2613) * IGNITE-19217 Implement foreign keys query for ODBC (apache#2692) * IGNITE-20512 Remove port range from HTTP server (apache#2673) * Remove port range from HTTP server configuration * Update junit5 version to avoid jar hell. * IGNITE-20657 At the checkpoint ArrayIndexOutOfBoundsException (apache#2696) * IGNITE-20444 Sql. Add restrictions for execution tx related statements with single statement mode (apache#2683) * IGNITE-20116 Linearize storage updates with safeTime adjustment rules (apache#2689) * IGNITE-20636 Add to the MakeIndexAvailableCommand the ability to use only the indexId (apache#2691) * IGNITE-19219 Implement primary keys query in ODBC (apache#2699) * IGNITE-20366 testBatchReadPutConcurrently failed (apache#2694) * IGNITE-20435 Preserve key order in batch opperations (deleteAll, deleteAllExact, insertAll) (apache#2664) * IGNITE-20385 Sql. Fixed handling of NodeLeftException (apache#2622) * IGNITE-20672 Broke compilation (apache#2705) * IGNITE-20530 Start building indexes for write-only indexes (apache#2704) * IGNITE-20659 Placement driver do not create a lease (apache#2700) * IGNITE-20668 Increase wait after a DDL to account for idle safe-time propagation period (apache#2703) * IGNITE-20522 Create the default user 'Ignite' (apache#2684) * Ignite/ignite default user is added to the configuration if the user did not specify one * security.authentication.enabled -> security.enabled * We didn't have the ability to set dynamic defaults in the current implementation of configuration before this patch. Moreover, we don't have the ability to set a default value for NamedConfigValue at all. Now you can add the code to the configuration module if you want to add some defaults based on user-provided values or other external conditions. * IGNITE-20616 ensureReplicaIsPrimary should use getPrimaryReplica instead of awaitPrimaryReplica (apache#2701) * IGNITE-20671 Sql. Fixed ItSqlApiTest#ddl test (apache#2708) * IGNITE-20677 Sql. Improve error reporting in assertThrowsSqlException (apache#2707) * IGNITE-20600 Sql. Fix a message of an error, which occurs while updating primary key column (apache#2695) * IGNITE-20478 Sql. Get rid of UNSPECIFIED_VALUE_PLACEHOLDER (apache#2671) * IGNITE-20430 Got rid of unused set and fixed replica waiters removal (apache#2604) * IGNITE-20454 Sql. Added a callback that is notified when data prefetching is complete (apache#2674) * IGNITE-20002 Implement durable unlock on primary partition re-election (apache#2697) * IGNITE-20630 Select only available nodes for deployment unit download (apache#2713) * IGNITE-20693 Fixed NPE in placement driver actor on deactivation (apache#2718) * IGNITE-20695 Cleanup resource (apache#2723) * IGNITE-20545 Improve logging in AbstractRpcTest (apache#2717) * IGNITE-20395 Clean up write intents for RW transaction on primary (apache#2679) * IGNITE-20699 Decrease idle safe time propagation period in tests (apache#2730) * IGNTIE-20629 Exclude ODBC build from assemble pipeline by default (apache#2706) * IGNITE-20706 Rename CatalogSchemaManager to SchemaManager (apache#2733) * IGNITE-20702 Fix NPE in ReplicaManager.onReplicaMessageReceived (apache#2731) * IGNITE-20704 Add methods to fsync files and directories (apache#2735) * IGNITE-20703 LeaseUpdaterTest causes a NullPointerException (apache#2732) * IGNITE-20599 Implement 'NOT_TOMBSTONE' operation in the meta storage dsl. (apache#2722) * IGNITE-20042 Check table existence before executing each operation in an RW transaction (apache#2721) * IGNITE-20576 Fix testGetReturningTupleWithUnknownSchemaRequestsNewSchema (apache#2738) * IGNITE-20359 Expose storage profiles as a node attribute (apache#2711) * IGNITE-20439 Sql. Support multiple schemas in CatalogSqlSchemaManager (apache#2719) * IGNITE-20311 Sql. Fix behaviour of ROUND function (apache#2690) * IGNITE-20684 Use proper sync method (apache#2715) * IGNITE-20670 Extract deployment code integration test to related module (apache#2714) * IGNITE-20304 Sql. Documentation for system views module (apache#2726) * IGNITE-20644 Java thin: Fix error detection in doSchemaOutInOpAsync (apache#2739) Fix well-known error detection in `ClientTable` when `sendServerExceptionStackTraceToClient` is enabled: `unwrapRootCause` did not work, the important exception can be on any level. * IGNITE-20498 Fix potential catalog version order violations (apache#2734) * IGNITE-20560 Remove the field out of sync with the map (apache#2660) * IGNITE-20099 Update okhttp version (apache#2746) * IGNITE-20720 Move ClusterPerTestIntegrationTest and ClusterPerClassIntegrationTest to Test Fixtures (apache#2744) * IGNITE-20618 Sql. Degradation of SELECT operations performance over time (apache#2728) * IGNITE-20726: Fix incorrect link that mentions a closed ticket. (apache#2745) * IGNITE-20727 Pass schema version in each read/write ReplicaRequest (apache#2747) * IGNITE-20561 Change condition for DistributionZonesUtil#triggerKeyConditionForZonesChanges to solve inconsistency issues (apache#2743) * IGNITE-20734 Fixed a link to cli doc (apache#2750) * IGNITE-20739 fix compilation after merge of IGNITE-20561 (apache#2753) * IGNITE-20624 Fix race between getting logical topology and mapping fragments (apache#2710) --------- Signed-off-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> Co-authored-by: Mirza Aliev <alievmirza@gmail.com> Co-authored-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> Co-authored-by: Kirill Tkalenko <tkalkirill@yandex.ru> Co-authored-by: korlov42 <korlov@gridgain.com> Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com> Co-authored-by: Max Zhuravkov <shhwwa@gmail.com> Co-authored-by: Cyrill <cyrill.sizov@gmail.com> Co-authored-by: Ivan Gagarkin <gagarkin.iiu@gmail.com> Co-authored-by: Vladislav Pyatkov <vldpyatkov@gmail.com> Co-authored-by: Igor Sapego <isapego@apache.org> Co-authored-by: Alexander Lapin <lapin1702@gmail.com> Co-authored-by: ygerzhedovich <41903880+ygerzhedovich@users.noreply.github.com> Co-authored-by: Aleksandr Pakhomov <apkhmv@gmail.com> Co-authored-by: Pavel Pereslegin <xxtern@gmail.com> Co-authored-by: Evgeniy Stanilovskiy <stanilovsky@gmail.com> Co-authored-by: Denis Chudov <moonglloom@gmail.com> Co-authored-by: Mikhail <Pochatkin@users.noreply.github.com> Co-authored-by: Vadim Pakhnushev <8614891+valepakh@users.noreply.github.com> Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com> Co-authored-by: Pavel Tupitsyn <ptupitsyn@apache.org> Co-authored-by: Andrew V. Mashenkov <AMashenkov@users.noreply.github.com> Co-authored-by: IgGusev <igusev@gridgain.com>

tkalkirill added 4 commits October 11, 2023 17:04

IGNITE-20620 wip

e90648c

IGNITE-20620 wip

d8440e8

IGNITE-20620 wip

f4eb671

IGNITE-20620 wip

1a65e10

rpuch reviewed Oct 12, 2023

View reviewed changes

IGNITE-20620 after review #1.0

1902d40

rpuch reviewed Oct 12, 2023

View reviewed changes

...c/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogSortedIndexDescriptor.java Outdated Show resolved Hide resolved

rpuch approved these changes Oct 12, 2023

View reviewed changes

IGNITE-20620 after review #1.1

7840586

tkalkirill merged commit 0597d0c into apache:main Oct 12, 2023
1 check passed

tkalkirill deleted the ignite-20620 branch October 12, 2023 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-20620 Add index availability command to catalog #2680

IGNITE-20620 Add index availability command to catalog #2680

tkalkirill commented Oct 11, 2023

rpuch Oct 12, 2023

tkalkirill Oct 12, 2023

rpuch Oct 12, 2023

rpuch Oct 12, 2023

tkalkirill Oct 12, 2023

rpuch Oct 12, 2023

tkalkirill Oct 12, 2023

rpuch Oct 12, 2023

tkalkirill Oct 12, 2023

rpuch Oct 12, 2023

tkalkirill Oct 12, 2023

rpuch left a comment

IGNITE-20620 Add index availability command to catalog #2680

IGNITE-20620 Add index availability command to catalog #2680

Conversation

tkalkirill commented Oct 11, 2023

The Review Checklist

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpuch left a comment

Choose a reason for hiding this comment