-
Notifications
You must be signed in to change notification settings - Fork 40
Create index if not exists in repairTable of CosmosAdmin and DynamoAdmin #2800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
948822d
5fec84d
8f3d453
0d11898
da032bb
82354b9
3700df9
7b700b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1032,6 +1032,46 @@ columnName, getFullTableName(namespace, table)), | |||||
| TableMetadata.newBuilder(tableMetadata).addSecondaryIndex(columnName).build()); | ||||||
| } | ||||||
|
|
||||||
| private boolean rawIndexExists(String nonPrefixedNamespace, String table, String indexName) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I felt
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your comment! It seems that the prefix "Raw" is used to refer to things not managed by ScalarDB metadata, such as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! OK, let's keep it as is for this PR. @brfrn169 It's very minor, but I think |
||||||
| Namespace namespace = Namespace.of(namespacePrefix, nonPrefixedNamespace); | ||||||
| String globalIndexName = | ||||||
| String.join( | ||||||
| ".", | ||||||
| getFullTableName(namespace, table), | ||||||
| DynamoAdmin.GLOBAL_INDEX_NAME_PREFIX, | ||||||
| indexName); | ||||||
| int retryCount = 0; | ||||||
| try { | ||||||
| while (true) { | ||||||
| DescribeTableResponse response = | ||||||
| client.describeTable( | ||||||
| DescribeTableRequest.builder() | ||||||
| .tableName(getFullTableName(namespace, table)) | ||||||
| .build()); | ||||||
| GlobalSecondaryIndexDescription description = | ||||||
| response.table().globalSecondaryIndexes().stream() | ||||||
| .filter(d -> d.indexName().equals(globalIndexName)) | ||||||
| .findFirst() | ||||||
| .orElse(null); | ||||||
| if (description == null) { | ||||||
| return false; | ||||||
| } | ||||||
| if (description.indexStatus() == IndexStatus.ACTIVE) { | ||||||
| return true; | ||||||
| } | ||||||
| if (retryCount++ >= MAX_RETRY_COUNT) { | ||||||
| throw new IllegalStateException( | ||||||
| String.format( | ||||||
| "Waiting for the secondary index %s on the %s table to be active failed", | ||||||
| indexName, getFullTableName(namespace, table))); | ||||||
| } | ||||||
| Uninterruptibles.sleepUninterruptibly(waitingDurationSecs, TimeUnit.SECONDS); | ||||||
| } | ||||||
| } catch (ResourceNotFoundException e) { | ||||||
| return false; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void waitForIndexCreation(Namespace namespace, String table, String columnName) | ||||||
| throws ExecutionException { | ||||||
| try { | ||||||
|
|
@@ -1366,6 +1406,11 @@ public void repairTable( | |||||
| throws ExecutionException { | ||||||
| try { | ||||||
| createTableInternal(nonPrefixedNamespace, table, metadata, true, options); | ||||||
| for (String indexColumnName : metadata.getSecondaryIndexNames()) { | ||||||
| if (!rawIndexExists(nonPrefixedNamespace, table, indexColumnName)) { | ||||||
| createIndex(nonPrefixedNamespace, table, indexColumnName, options); | ||||||
| } | ||||||
| } | ||||||
| } catch (RuntimeException e) { | ||||||
| throw new ExecutionException( | ||||||
| String.format( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -893,6 +893,13 @@ public void repairTable_withStoredProcedure_ShouldNotAddStoredProcedure() | |
| when(scripts.getStoredProcedure(CosmosAdmin.STORED_PROCEDURE_FILE_NAME)) | ||
| .thenReturn(storedProcedure); | ||
|
|
||
| // Existing container properties | ||
| CosmosContainerResponse response = mock(CosmosContainerResponse.class); | ||
| when(database.createContainerIfNotExists(table, "/concatenatedPartitionKey")) | ||
| .thenReturn(response); | ||
| CosmosContainerProperties properties = mock(CosmosContainerProperties.class); | ||
| when(response.getProperties()).thenReturn(properties); | ||
|
|
||
|
Comment on lines
+896
to
+902
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lines are prepared to handle the addition of |
||
| // Act Assert | ||
| admin.repairTable(namespace, table, tableMetadata, Collections.emptyMap()); | ||
|
|
||
|
|
@@ -943,6 +950,13 @@ public void repairTable_withoutStoredProcedure_ShouldCreateStoredProcedure() | |
| when(cosmosException.getStatusCode()).thenReturn(404); | ||
| when(storedProcedure.read()).thenThrow(cosmosException); | ||
|
|
||
| // Existing container properties | ||
| CosmosContainerResponse response = mock(CosmosContainerResponse.class); | ||
| when(database.createContainerIfNotExists(table, "/concatenatedPartitionKey")) | ||
| .thenReturn(response); | ||
| CosmosContainerProperties properties = mock(CosmosContainerProperties.class); | ||
| when(response.getProperties()).thenReturn(properties); | ||
|
|
||
|
Comment on lines
+953
to
+959
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
| // Act | ||
| admin.repairTable(namespace, table, tableMetadata, Collections.emptyMap()); | ||
|
|
||
|
|
@@ -996,6 +1010,13 @@ public void repairTable_ShouldCreateTableIfNotExistsAndUpsertMetadata() | |
| when(cosmosException.getStatusCode()).thenReturn(404); | ||
| when(storedProcedure.read()).thenThrow(cosmosException); | ||
|
|
||
| // Existing container properties | ||
| CosmosContainerResponse response = mock(CosmosContainerResponse.class); | ||
| when(database.createContainerIfNotExists(table, "/concatenatedPartitionKey")) | ||
| .thenReturn(response); | ||
| CosmosContainerProperties properties = mock(CosmosContainerProperties.class); | ||
| when(response.getProperties()).thenReturn(properties); | ||
|
|
||
|
Comment on lines
+1013
to
+1019
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
| // Act | ||
| admin.repairTable(namespace, table, tableMetadata, Collections.emptyMap()); | ||
|
|
||
|
|
@@ -1024,6 +1045,49 @@ public void repairTable_ShouldCreateTableIfNotExistsAndUpsertMetadata() | |
| verify(scripts).createStoredProcedure(any()); | ||
| } | ||
|
|
||
| @Test | ||
| public void repairTable_WhenTableAlreadyExistsWithoutIndex_ShouldCreateIndex() | ||
| throws ExecutionException { | ||
| // Arrange | ||
| String namespace = "ns"; | ||
| String table = "tbl"; | ||
| TableMetadata tableMetadata = | ||
| TableMetadata.newBuilder() | ||
| .addColumn("c1", DataType.INT) | ||
| .addColumn("c2", DataType.TEXT) | ||
| .addColumn("c3", DataType.BIGINT) | ||
| .addPartitionKey("c1") | ||
| .addSecondaryIndex("c3") | ||
| .build(); | ||
| when(client.getDatabase(namespace)).thenReturn(database); | ||
| when(database.getContainer(table)).thenReturn(container); | ||
| // Metadata container | ||
| CosmosContainer metadataContainer = mock(CosmosContainer.class); | ||
| CosmosDatabase metadataDatabase = mock(CosmosDatabase.class); | ||
| when(client.getDatabase(METADATA_DATABASE)).thenReturn(metadataDatabase); | ||
| when(metadataDatabase.getContainer(CosmosAdmin.TABLE_METADATA_CONTAINER)) | ||
| .thenReturn(metadataContainer); | ||
| // Stored procedure exists | ||
| CosmosScripts scripts = mock(CosmosScripts.class); | ||
| when(container.getScripts()).thenReturn(scripts); | ||
| CosmosStoredProcedure storedProcedure = mock(CosmosStoredProcedure.class); | ||
| when(scripts.getStoredProcedure(CosmosAdmin.STORED_PROCEDURE_FILE_NAME)) | ||
| .thenReturn(storedProcedure); | ||
| // Existing container properties | ||
| CosmosContainerResponse response = mock(CosmosContainerResponse.class); | ||
| when(database.createContainerIfNotExists(table, "/concatenatedPartitionKey")) | ||
| .thenReturn(response); | ||
| CosmosContainerProperties properties = mock(CosmosContainerProperties.class); | ||
| when(response.getProperties()).thenReturn(properties); | ||
|
|
||
| // Act | ||
| admin.repairTable(namespace, table, tableMetadata, Collections.emptyMap()); | ||
|
|
||
| // Assert | ||
| verify(container, times(1)).replace(properties); | ||
| verify(properties, times(1)).setIndexingPolicy(any(IndexingPolicy.class)); | ||
| } | ||
|
|
||
| @Test | ||
| public void addNewColumnToTable_ShouldWorkProperly() throws ExecutionException { | ||
| // Arrange | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package com.scalar.db.api; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatCode; | ||
|
|
||
| import com.scalar.db.exception.storage.ExecutionException; | ||
| import com.scalar.db.io.DataType; | ||
|
|
@@ -212,6 +213,21 @@ public void repairTable_ForNonExistingTableButExistingMetadata_ShouldCreateTable | |
| assertThat(admin.getTableMetadata(getNamespace(), getTable())).isEqualTo(getTableMetadata()); | ||
| } | ||
|
|
||
| @Test | ||
| public void | ||
| repairTable_WhenTableAlreadyExistsWithoutIndexAndMetadataSpecifiesIndex_ShouldCreateIndex() | ||
| throws Exception { | ||
| // Arrange | ||
| admin.dropIndex(getNamespace(), getTable(), COL_NAME5); | ||
|
|
||
| // Act | ||
| admin.repairTable(getNamespace(), getTable(), getTableMetadata(), getCreationOptions()); | ||
|
Comment on lines
+220
to
+224
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prepare a situation where the index does not exist in the actual table, but the index is specified in the table metadata given when |
||
|
|
||
| // Assert | ||
| assertThatCode(() -> admin.dropIndex(getNamespace(), getTable(), COL_NAME5)) | ||
| .doesNotThrowAnyException(); | ||
|
Comment on lines
+227
to
+228
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the fact that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brfrn169 |
||
| } | ||
|
|
||
| @Test | ||
| public void repairNamespace_ForExistingNamespace_ShouldDoNothing() throws Exception { | ||
| // Act | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.