Skip to content
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

fix(mongo): retry only 3 times if ensuring indexes fails #3272

Merged
merged 7 commits into from Jul 2, 2022

Conversation

jagzmz
Copy link
Contributor

@jagzmz jagzmz commented Jul 1, 2022

Closes #3261

@B4nan I am not sure how would I reproduce the error on tests, they are not complete as of now, any suggestions ?

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #3272 (20722a2) into master (972e5ba) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #3272   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          194       194           
  Lines        12094     12097    +3     
  Branches      2806      2807    +1     
=========================================
+ Hits         12094     12097    +3     
Impacted Files Coverage Δ
packages/mongodb/src/MongoSchemaGenerator.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 972e5ba...20722a2. Read the comment docs.

@B4nan
Copy link
Member

B4nan commented Jul 1, 2022

Here it should be enough if you define the unique constraint, but dont create it right ahead, create the two rows, and then call ensureIndexes() afterwards. First comment out the changes you made to verify such test will fail, then see how your fix is working.

Note that to have that test "work" on every run, you will also need to wipe the collections so the indexes also get wiped.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! just few nits before we merge

tests/features/schema-generator/GH3261.test.ts Outdated Show resolved Hide resolved
tests/features/schema-generator/GH3261.test.ts Outdated Show resolved Hide resolved
tests/features/schema-generator/GH3261.test.ts Outdated Show resolved Hide resolved
tests/features/schema-generator/GH3261.test.ts Outdated Show resolved Hide resolved
@@ -74,7 +74,7 @@ export class MongoSchemaGenerator extends AbstractSchemaGenerator<MongoDriver> {
await Promise.all(promises);
}

async ensureIndexes(options: EnsureIndexesOptions = {}): Promise<void> {
async ensureIndexes(options: EnsureIndexesOptions = {}, retryLimit = 3): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, lets not introduce another parameter, we already have the options there, so lets just add the retryLimit inside that

Suggested change
async ensureIndexes(options: EnsureIndexesOptions = {}, retryLimit = 3): Promise<void> {
async ensureIndexes(options: EnsureIndexesOptions = {}): Promise<void> {
options.retryLimit ??= 3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

Comment on lines 106 to 110
if (retryLimit === 1) {
const failedIndexes = res.filter(r => r.status === 'rejected').map((r: Dictionary, id) => `${promises[id][0]} - ${r.reason}`);
throw new Error(`Failed to create indexes: ${failedIndexes.join(', ')}`);
}
await this.ensureIndexes({ retry: collectionsWithFailedIndexes }, retryLimit - 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (retryLimit === 1) {
const failedIndexes = res.filter(r => r.status === 'rejected').map((r: Dictionary, id) => `${promises[id][0]} - ${r.reason}`);
throw new Error(`Failed to create indexes: ${failedIndexes.join(', ')}`);
}
await this.ensureIndexes({ retry: collectionsWithFailedIndexes }, retryLimit - 1);
if (options.retryLimit === 1) {
const failedIndexes = res.filter(r => r.status === 'rejected').map((r: Dictionary, id) => `${promises[id][0]} - ${r.reason}`);
throw new Error(`Failed to create indexes: ${failedIndexes.join(', ')}`);
}
await this.ensureIndexes({
retry: collectionsWithFailedIndexes,
retryLimit: retryLimit - 1,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@jagzmz jagzmz requested a review from B4nan July 2, 2022 03:53
@B4nan B4nan changed the title fix(mongo): add retry limit to 3 if indexing fails while adding invalid indexes fix(mongo): retry only 3 times if ensuring indexes fails Jul 2, 2022
@B4nan B4nan merged commit 299a028 into mikro-orm:master Jul 2, 2022
@B4nan
Copy link
Member

B4nan commented Jul 2, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MongoDb: ensureIndexes() goes into infinite loop, when there is an error while creating indexes
3 participants