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

Fixes Issue #610 #611

Merged
merged 6 commits into from Jan 27, 2022
Merged

Conversation

zachlankton
Copy link
Contributor

@zachlankton zachlankton commented Nov 14, 2021

This pull request fixes issue #610 as well as passes all legacy and non legacy tests (without the OTTOMAN_LEGACY_TEST=1 set).

Intermittently there would be LCB_ERR_KEYSPACE_NOT_FOUND when creating new collections and indexes. This would happen because sometimes the ensureCollections would return before the collections were actually ready. Additionally sometimes there would be Query Planning Failures when running on freshly created models because the ensureIndexes would not return the indexOnlinePromise unless indexReadyHooks were defined.

The changes in this pull request refactor some logic in the ensureCollections and ensureIndexes functions. It now guarantees that the collections and the indexes will be ready as described in the documentation.

Copy link
Contributor

@httpJunkie httpJunkie left a comment

Choose a reason for hiding this comment

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

Thanks for the Pull Request and the detailed information about your findings and tests. I will approve and merge this PR.

@httpJunkie
Copy link
Contributor

I've ran into some failed tests on both test and test:legacy so for this reason I am going to try and get one of our maintainers (Lino) to take a look at the PR and approve. But thank you for the PR, we are working on it.

@zachlankton
Copy link
Contributor Author

zachlankton commented Nov 17, 2021

Sounds good. Let me know if I can help! Will the github action run to test this?

@httpJunkie httpJunkie requested review from httpJunkie and removed request for gsi-alejandro December 20, 2021 15:35
Copy link
Contributor

@httpJunkie httpJunkie left a comment

Choose a reason for hiding this comment

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

We will be reviewing this PR this week. We may make changes to or replace this PR with something else that works the same. Either way, we will resolve this issue.

@zachlankton
Copy link
Contributor Author

Quick update: I have been using the code in this pull request for about a month now and have not found any new issues! 😊

@ejscribner
Copy link
Collaborator

Thanks for the update @zachlankton!! I'll see what we can do about getting this into the next release. I appreciate the contribution!

@ejscribner
Copy link
Collaborator

@zachlankton We've updated the testing workflow to include PRs from branches in forks. Could you amend your last commit (just change a single character or something) to trigger the tests?

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #611 (fd01008) into master (1fb8ced) will decrease coverage by 0.21%.
The diff coverage is 87.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
- Coverage   95.21%   95.00%   -0.22%     
==========================================
  Files          93       93              
  Lines        2487     2580      +93     
  Branches      566      574       +8     
==========================================
+ Hits         2368     2451      +83     
- Misses        111      122      +11     
+ Partials        8        7       -1     
Impacted Files Coverage Δ
src/exceptions/exceptions.ts 100.00% <ø> (ø)
src/handler/find/find-by-id-options.ts 100.00% <ø> (ø)
src/handler/find/find-options.ts 100.00% <ø> (ø)
src/model/utils/update-refdoc-indexes.ts 54.54% <0.00%> (ø)
src/query/index.ts 100.00% <ø> (ø)
src/query/query.ts 87.41% <20.00%> (-2.45%) ⬇️
src/utils/parse-errors.ts 16.66% <22.22%> (-83.34%) ⬇️
src/model/index/n1ql/ensure-n1ql-indexes.ts 83.75% <57.89%> (+2.05%) ⬆️
src/model/create-model.ts 88.61% <66.66%> (-5.08%) ⬇️
src/model/utils/store-life-cycle.ts 92.68% <85.71%> (-7.32%) ⬇️
... and 30 more

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 a5a52f3...fd01008. Read the comment docs.

@zachlankton
Copy link
Contributor Author

Looks like the tests are passing, but code coverage is failing for files and lines of code that this PR did not touch. Looks like the same thing is happening in PR #614

@zachlankton
Copy link
Contributor Author

Actually, just noticed codecov is comparing this pull request to an old commit from August — 1fb8ced

@ejscribner
Copy link
Collaborator

I believe this is ok for now, I'll touch base with the team tomorrow. Thanks so much @zachlankton!

@ejscribner
Copy link
Collaborator

Hey @zachlankton just a quick update on this: we're planning on including it in our next release (sometime next week I believe)! Thanks again for being a part of our growing community. Cheers!

@ejscribner ejscribner merged commit 85005c1 into couchbaselabs:master Jan 27, 2022
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.

None yet

3 participants