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.

Sign up for GitHub

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

Jump to bottom

fix: add transaction on mongo #10491

Merged
merged 1 commit into from
Jan 30, 2025
Merged

fix: add transaction on mongo #10491

merged 1 commit into from
Jan 30, 2025

Conversation

guillaumelamirand
Copy link
Contributor

@guillaumelamirand guillaumelamirand commented Jan 21, 2025 •
edited by graviteeio
Loading

Description

A init script to init replicatset.

Additional context

📚  View the storybook of this branch here

@guillaumelamirand guillaumelamirand requested a review from a team as a code owner January 21, 2025 14:41
jgiovaresco
jgiovaresco approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@jgiovaresco jgiovaresco left a comment

Choose a reason for hiding this comment

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

We can use the tag run-e2e-tests on the PR to trigger the e2e to ensure that everything is good

@guillaumelamirand
Copy link
Contributor Author

guillaumelamirand commented Jan 21, 2025 •
edited
Loading

We can use the tag run-e2e-tests on the PR to trigger the e2e to ensure that everything is good

Before doing it, I needed to wait until the first build was successful.

Edit: I knew I should have wait...

@phiz71 phiz71 force-pushed the add-init-mongodb-script branch from db2b91c to be2041c Compare January 22, 2025 07:53
@guillaumelamirand guillaumelamirand changed the title fix: add missing init.js script for mongo fix: add transaction on mongo Jan 22, 2025
@guillaumelamirand guillaumelamirand force-pushed the add-init-mongodb-script branch 3 times, most recently from 2a6ec5e to 18886a7 Compare January 22, 2025 16:39
@guillaumelamirand guillaumelamirand force-pushed the add-init-mongodb-script branch 2 times, most recently from 70f3e0d to 18f8e64 Compare January 22, 2025 16:44
@gravitee-io gravitee-io deleted a comment from cypress bot Jan 22, 2025
@guillaumelamirand guillaumelamirand marked this pull request as draft January 22, 2025 17:04
@guillaumelamirand guillaumelamirand marked this pull request as ready for review January 22, 2025 17:31
@cypress Cypress
Copy link

cypress bot commented Jan 22, 2025 •
edited
Loading

APIM UI Tests    Run #3628

Run Properties:  status check passed Passed #3628  •  git commit 585cf1cdd4: feat: enable mongodb transaction manager
Project APIM UI Tests
Branch Review add-init-mongodb-script
Run status status check passed Passed #3628
Run duration 07m 08s
Commit git commit 585cf1cdd4: feat: enable mongodb transaction manager
Committer Guillaume Lamirand
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 8
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 82
View all changes introduced in this branch ↗︎

jgiovaresco
jgiovaresco reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@jgiovaresco jgiovaresco left a comment

Choose a reason for hiding this comment

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

I think we need to also add the retry annotation to https://github.com/gravitee-io/gravitee-api-management/blob/master/gravitee-apim-rest-api/gravitee-apim-rest-api-service/src/main/java/io/gravitee/apim/core/UseCase.java#L52

and for the future I'm wondering it we need to add a CI pipeline to test the E2E in non transactional mode: have 2 run for MongoDB like we have multiple run for JDBC cc @phiz71

@guillaumelamirand
Copy link
Contributor Author

I think we need to also add the retry annotation to https://github.com/gravitee-io/gravitee-api-management/blob/master/gravitee-apim-rest-api/gravitee-apim-rest-api-service/src/main/java/io/gravitee/apim/core/UseCase.java#L52

and for the future I'm wondering it we need to add a CI pipeline to test the E2E in non transactional mode: have 2 run for MongoDB like we have multiple run for JDBC cc @phiz71

I am wondering, if it is a good idea to have to different kind of annotation for Transaction, especialy because usecase uses service which are transactionnal and already in retry mode...

Don't know about the two kind of tests because the non transactional shouldn't be advice to anyone

@guillaumelamirand guillaumelamirand force-pushed the add-init-mongodb-script branch 2 times, most recently from 943ab86 to bd6300d Compare January 27, 2025 15:47
feat: enable mongodb transaction manager
d139585 
   - add option repository.mongodb.transactional to disable transaction (enable by default)
   - add repliSet option on all docker-compose
   - add retryable framework to retry TransientDataAccessException and some NonTransientDataAccessException including WriteConflict error coming from mongodb transaction
@guillaumelamirand guillaumelamirand enabled auto-merge (rebase) January 30, 2025 08:06
@guillaumelamirand guillaumelamirand merged commit 8be0838 into master Jan 30, 2025
37 of 39 checks passed
@guillaumelamirand guillaumelamirand deleted the add-init-mongodb-script branch January 30, 2025 08:23
@guillaumelamirand guillaumelamirand mentioned this pull request Jan 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhaeyaert jhaeyaert jhaeyaert approved these changes

@jgiovaresco jgiovaresco jgiovaresco approved these changes

Assignees
No one assigned
Labels
run-e2e-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guillaumelamirand @jgiovaresco @jhaeyaert