retry dictionary registration#201
Conversation
| 409: | ||
| $ref: '#/components/responses/StatusConflict' |
There was a problem hiding this comment.
if registering the same Dictionary for the category then it throws a 409 error, unless the "force" flag is true.
| - name: force | ||
| description: Re-registers the current active dictionary on the category and retries the data migration. | ||
| in: query | ||
| required: false | ||
| schema: | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
Can you add more detail about this behaviour? I don't know what re-registering a dictionary will do. Is this the same as initiating a migration? If the dictionary version is the same as the current dictionary will it run the migration with the intention of revalidating the data (should have no impact on the current data state)?
There was a problem hiding this comment.
description updated to Runs dictionary registration and migration again for this category, even if the same dictionary is already registered. Use this only when a previous migration ended unexpectedly and you must rerun both steps.
|
|
||
| If a category is already using the same dictionary name and version, registration returns `409 Conflict`. | ||
|
|
||
| Set the `force` query parameter to `true` to allow re-registration and trigger a migration (or retry) to revalidate existing category data against that dictionary. |
There was a problem hiding this comment.
Repeating my comment since this is the full documentation about registering dictionaries:
Can you add more detail about this behaviour? I don't know what re-registering a dictionary will do. Is this the same as initiating a migration? If the dictionary version is the same as the current dictionary will it run the migration with the intention of revalidating the data (should have no impact on the current data state)?
| const newMigration: NewDictionaryMigration = { | ||
| categoryId, | ||
| fromDictionaryId, | ||
| fromDictionaryId: fromDictionaryId ?? toDictionaryId, |
There was a problem hiding this comment.
This is confusing to read - perhaps we should leave it as required and the caller can indicate that fromDictionaryId value should be the same value as toDictionaryId. I would not expect the migration service to assume that if from value is not provided you reuse the to value.
There was a problem hiding this comment.
code updated to make fromDictionaryId required
| if (forceRegistration) { | ||
| logger.info( | ||
| LOG_MODULE, | ||
| `Force flag is true, initiating migration for Category '${foundCategory.name}' | ||
| with Dictionary '${savedDictionary.name}' version '${savedDictionary.version}'`, | ||
| ); | ||
|
|
||
| const resultMigration = await initiateMigration({ | ||
| categoryId: foundCategory.id, | ||
| toDictionaryId: savedDictionary.id, | ||
| userName: username || '', | ||
| }); | ||
|
|
||
| if (!resultMigration.success) { | ||
| const errorMessage = `Failed to initiate migration for category '${categoryName}' with error: ${resultMigration.data}`; | ||
| logger.error(LOG_MODULE, errorMessage); | ||
| throw new Error(errorMessage); | ||
| } | ||
|
|
||
| return { dictionary: savedDictionary, category: foundCategory, migrationId: resultMigration.data }; | ||
| } | ||
|
|
||
| throw new StatusConflict( | ||
| `Category '${categoryName}' with Dictionary '${savedDictionary.name}' version '${savedDictionary.version}' already exists`, | ||
| ); |
There was a problem hiding this comment.
This is not very DRY, a lot of repeated code from the else block. The only difference is omitting the fromDictionaryId and changed log statements. Is there a way to write this that won't open us up to a future code change that is only made in one branch of this if statement?
There was a problem hiding this comment.
refactored this code to make it DRY and reusing the initiateMigration logic.
|
PR updated to address feedback received. Ready for code review |
|
Could you add a bit more description to the PR to explain "what happens if the force flag is used when migration is not in failed state"? For example, does it interrupt a migration thats in progress, does it repeat a completed migration. Any behaviours that are modified by this force flag need to be claerly stated. |
|
|
||
| Category groups data that is related and shares the same data structure, for that reason, a category must be associated to a registered dictionary. Over time, if the dictionary requires an update, the category needs to be updates accordingly, See [Dictionary Migration](#dictionary-migration) for more details. | ||
|
|
||
| If a category is already using the same dictionary name and version, registration returns `409 Conflict` by default. If the `force` query parameter is set to `true`, Lyric retries only when the same dictionary is already registered and the latest migration previously failed. If no prior failed migration exists, the `force` flag is ignored. |
There was a problem hiding this comment.
updated doc to describe force flag behaviour
|
|
||
| return { dictionary: savedDictionary, category: foundCategory }; | ||
| // check last migration of this category to find if it failed, if it failed and forceRegistration is true, | ||
| // we will re-initiate the migration with the new dictionary | ||
| const activeMigration = await getMigrationsByCategoryId(foundCategory.id, { pageSize: 1, page: 1 }); | ||
|
|
||
| if ( | ||
| forceRegistration && | ||
| activeMigration.result.length === 1 && | ||
| activeMigration.result.at(0)?.status === 'FAILED' | ||
| ) { |
There was a problem hiding this comment.
a migration can be re-run only when last migration has FAILED and force flag is set to true
|
PR updated, changes on readme and openapi doc, to indicate |
2 participants
Description
It enables the possibility to retry "failed" Dictionary migration by adding a
forceflag on the Dictionary Registration endpoint.Details:
POST /dictionary/registerendpoint to accept an optional query paramforceto Re-register the current active dictionary on the category and retries the data migration.Related tickets:
This PR depends on: