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

3343: add multi mongo db support #4529

Merged
merged 1 commit into from Feb 6, 2020
Merged

Conversation

dufoli
Copy link
Contributor

@dufoli dufoli commented Oct 12, 2019

fix #3343

GOAL:

quarkus.mongodb.cluster1.connection-string = mongodb://mongo1:27017,mongo2:27017
quarkus.mongodb.cluster2.connection-string = mongodb://mongo1:27017,mongo2:27017

And then use annotations above the instances of MongoClient to indicate which instance we are going to use (example imaginary @connection annotation used):


@Connection("cluster1")
@Inject
ReactiveMongoClient mongoClientCluster1

I reuse(copy) a lot of code from agroal extention to have same structure. when I do not set codecs, it work well but codec test is failing. I need help to understand how to load codec during runtime and not deployment.

previous code: load mongoClient during runtime build step
new code: create producer for each named connection (like agroal extention) and buildstep during compile time.

depend of #5387. Have to wait that it land on master

@geoand
Copy link
Contributor

geoand commented Oct 13, 2019

Seems like some of the tests are failing.

@dufoli would you like to give some details about this PR?

@dufoli
Copy link
Contributor Author

dufoli commented Oct 13, 2019

@geoand I have fixed tests and many tests succeed!:

  • BasicInteractionTest
  • ListDatabaseTest
  • DatabaseRunCommandTest
  • DatabaseListTest
  • ReactiveMongoClientTest
  • ConnectionToReplicaSetTest

but a test is still failing: CollectionManagementTest
And I still need to build a unit test for multi connection.

I need help for the remaining test. IT fail with a timeout and before a really strange error on mongo side:[mongod output] : Permission denied Raw: [1570959216:414890][8624:140705864506960], file:WiredTiger.wt, WT_SESSION.checkpoint: __win_fs_rename, 105: C:\Users\olivier\Desktop\src\quarkus\extensions\mongodb-client\runtime\target\embedmongo-db-36eaa512-0929-4ce8-a36b-913061e2e295\WiredTiger.turtle.set to C:\Users\olivier\Desktop\src\quarkus\extensions\mongodb-client\runtime\target\embedmongo-db-36eaa512-0929-4ce8-a36b-913061e2e295\WiredTiger.turtle: file-rename: MoveFileExW: Acc▒s refus▒. [mongod output] : Permission denied [mongod output] 2019-10-13T11:33:36.419+0200 E STORAGE [WTCheckpointThread] WiredTiger error (13) [1570959216:418879][8624:140705864506960], file:WiredTiger.wt, WT_SESSION.checkpoint: __wt_turtle_update, 397: WiredTiger.turtle: fatal turtle file update error: Permission denied Raw: [1570959216:418879][8624:140705864506960], file:WiredTiger.wt, WT_SESSION.checkpoint: __wt_turtle_update, 397: WiredTiger.turtle: fatal turtle file update error: Permission denied [mongod output] 2019-10-13T11:33:36.420+0200 E STORAGE [WTCheckpointThread] WiredTiger error (-31804) [1570959216:419876][8624:140705864506960], file:WiredTiger.wt, WT_SESSION.checkpoint: __wt_panic, 523: the process must exit and restart: WT_PANIC: WiredTiger library panic Raw: [1570959216:419876][8624:140705864506960], file:WiredTiger.wt, WT_SESSION.checkpoint: __wt_panic, 523: the process must exit and restart: WT_PANIC: WiredTiger library panic

@dufoli
Copy link
Contributor Author

dufoli commented Oct 13, 2019

I need review and help to format. Because it seems to be ok but failing on code format....

@dufoli dufoli marked this pull request as ready for review October 14, 2019 02:56
@geoand
Copy link
Contributor

geoand commented Oct 14, 2019

Please run ./mvnw process-resources -f extensions/mongodb-client/pom.xml and once that's done commit the changes the formatter makes.

@Dufgui
Copy link
Contributor

Dufgui commented Oct 14, 2019

Yo bro. I like to see you code in Java ;)

@dufoli
Copy link
Contributor Author

dufoli commented Oct 14, 2019

good news, I have fixed the issue with codec. The mongodb test is succeed now but I have impact on panache over mongo. So I have to deep dive on it.

@geoand
Copy link
Contributor

geoand commented Oct 15, 2019

Good to hear!

@Dufgui
Copy link
Contributor

Dufgui commented Oct 15, 2019

For the panache case. I think you can get the default connection for a first shoot. And create an issue to treate it in a separate way.
I think the connection annotation must be put on the repository too. To get the good client in mongoOperation.

@geoand
Copy link
Contributor

geoand commented Oct 15, 2019

@loicmathieu since you have plenty of Mongo experience, would you like to check the PR out and let us know what you think?

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

I check globally the PR and ask some changes.
I don't check all the mongo connection initialization code, I hope you copy/paste it from the existing one :)

I can help for the Mongodb With Panache part if you didn't succeed to make it works.
After this PR is merged, we should update Mongodb With Panache to support multiple MongoClient, you can open an issue for this afterwards I'll take care of it.

gsmet
gsmet previously requested changes Oct 15, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Just to be sure this gets properly squashed before merging. Please dismiss once everything is squashed properly.

Thanks!

@dufoli dufoli force-pushed the mongo-cluster branch 3 times, most recently from 5f7a2d1 to 36ef743 Compare October 16, 2019 09:24
@dufoli
Copy link
Contributor Author

dufoli commented Oct 16, 2019

@geoand ping ;-) test has finished to run and now I get an error that I have not before my push. It do not succeed to get the producer to set up the config. I think it is because unremovable annotation make it failed to be created. I do not know why. producer is created in mongoClientProcessor. inside function createMongoClientProducerBean

@dufoli
Copy link
Contributor Author

dufoli commented Oct 16, 2019

I have test many way to generated bean unremovable.... but still failing.
my test was

  1. put annotation unremovable on producer class thanks to class cotrusctor FAILED
  2. create a build step with GeneratedBeanBuildItem as param and AdditionalBeanBuildItem. unremovalOf(getMongoClientProducerClassName()) ==> MongoClientProducer can not been found
  3. add it inside recorder with a callback function in created function ==> failed too
  4. add it right at the end of createMongoClientProducerBean ==> failed again
  5. change build return from beanContainerListeners to void and add a BuildProducer beanContainerListeners in order to additionalBeans.produce after beanContainerListeners.produce ==> failed.

IT was my last attempt... I still do not understand order of call and how be sure that generatedBean is produce and loaded before make it unremovable (and on the right side (deployment and not runtime).

@loicmathieu
Copy link
Contributor

@dufoli I'll checkout your branch today and have a look.
Maybe @cescoffier that implements the mongo-client extension can also have a look, maybe he will find the issue.

To debug if it's really a bean removal you can:

  • De-activate the removal of beans in the IT
  • Put Quarkus logs on DEBUG to see what happens on Arc

@geoand
Copy link
Contributor

geoand commented Oct 17, 2019

@dufoli have you looked at the output of the generated producer (it would be found under target/wiring-classes in a project that uses this extension) to see if there might be something obviously wrong with the produced code?

@loicmathieu
Copy link
Contributor

@geoand the extension development guide says that classes are generated into memory during augmentation, and that a java property needs to be specified to have them outputed into a directory: https://quarkus.io/guides/extension-authors-guide#troubleshooting-debugging-tips

@geoand
Copy link
Contributor

geoand commented Oct 17, 2019

@dufoli although not ideal, you could replace

AbstractMongoClientProducer producer = Arc.container().instance(AbstractMongoClientProducer.class).get();

with

Arc.container().instance(Class.forName("io.quarkus.mongodb.runtime.MongoClientProducer"));

and things will work (I tested).

I would suggest however extracting the common things into a MongoClientProducer interface, have the AbstractMongoClientProducer implement that interface and of course have MongoClientProducerImpl implement that abstract class (like you are doing now).

In that case, you'll (most likely) be able to pull the bean out of Arc using MongoClientProducer producer = Arc.container().instance(MongoClientProducer.class).get();

Does that make sense?

@geoand
Copy link
Contributor

geoand commented Oct 17, 2019

@geoand the extension development guide says that classes are generated into memory during augmentation, and that a java property needs to be specified to have them outputed into a directory: https://quarkus.io/guides/extension-authors-guide#troubleshooting-debugging-tips

@loicmathieu I am talking about doing the following:

mvn clean package -DskipTests -Dquarkus.build.skip=false inside the mongodb-client extension directory.
If you do that, you can see the produced bytecode in the target/wiring-classes directory. I have found it to be the easiest way to see the outcome of the bytecode I generate.

@geoand
Copy link
Contributor

geoand commented Oct 17, 2019

Also @loicmathieu TBH, the guide seems incorrect. That property is useful for dev-mode not for a typical mvn clean install. Would you be interested in verifying and providing a PR to update the guide?

@loicmathieu
Copy link
Contributor

@geoand yes, I'm interesting on improving the guide. You can open an issue and assign it to me.
I already have some guide improvements tasks on my todo list so it'll be easy to works on all of them in the same time.

@geoand
Copy link
Contributor

geoand commented Oct 17, 2019

@loicmathieu Thanks! Here you go: #4635

@dufoli dufoli changed the title 3343: add multi mongo db support WIP: 3343: add multi mongo db support Oct 18, 2019
@dufoli
Copy link
Contributor Author

dufoli commented Oct 18, 2019

@loicmathieu @geoand I have fixed the unremovable issue. Indeed I reuse the previous code with UnremovableBeanBuildItem because annotation unremovable are not parsed for generated code. And using other code do not work either. Anyway, I am now stuck on the

@dufoli
Copy link
Contributor Author

dufoli commented Jan 23, 2020

@geoand done!

@geoand
Copy link
Contributor

geoand commented Jan 23, 2020

Thank you!

We are having trouble with CI at the moment so it might be a while until we can get this in.

@dufoli
Copy link
Contributor Author

dufoli commented Feb 4, 2020

@gsmet @cescoffier @geoand Hello, code is ready. I have just rebase. Can you merge it ?

@geoand
Copy link
Contributor

geoand commented Feb 4, 2020

@dufoli Thanks. I need to take another look at this before merging. I'll let you know soon enough.

@dufoli
Copy link
Contributor Author

dufoli commented Feb 4, 2020

I have just fixed the test. It was because quarkus test is more strict now with java archive class to declared.
So I have added MongoTestBase to it.

@geoand
Copy link
Contributor

geoand commented Feb 5, 2020

@dufoli this may have been brought up before, but was is the reason that the a bunch of tests have moved?

@geoand geoand dismissed stale reviews from gsmet and cescoffier February 5, 2020 08:22

the commits have been properly squashed

@geoand
Copy link
Contributor

geoand commented Feb 5, 2020

I think that once the testing question I have has been addressed we can merge this

@dufoli
Copy link
Contributor Author

dufoli commented Feb 5, 2020

@geoand to be honnest, I do not remember... I can try to move it back to runtime part and check why. I think it was during a process of fixing. Not sure it was the root cause. AFAIK, I just do it because most of other extentions have it in deployment.

@geoand
Copy link
Contributor

geoand commented Feb 5, 2020

@dufoli please check and let me know. If we clear that up, I think we can merge this

@dufoli
Copy link
Contributor Author

dufoli commented Feb 5, 2020

@geoand I have retry to switch test folder back to runtime and update pom.cxml to reflect the change and I get these errors:
Caused by: javax.enterprise.inject.UnsatisfiedResolutionException: No bean found for required type [class io.quarkus.mongodb.NamedReactiveMongoClientConfigTest] and qualifiers [[]]
at io.quarkus.arc.impl.InstanceImpl.bean(InstanceImpl.java:151)

2020-02-05 22:44:15,939 WARN [io.qua.config] (main) Unrecognized configuration key "quarkus.mongodb.cluster1.connection-string" was provided; it will be ignored
2020-02-05 22:44:15,939 WARN [io.qua.config] (main) Unrecognized configuration key "quarkus.mongodb.cluster2.connection-string" was provided; it will be ignored

@dufoli
Copy link
Contributor Author

dufoli commented Feb 5, 2020

So I guess, it was done on purpose. I do not remember sadly the exact reason... It take too long time to review/merge sorry.

@geoand
Copy link
Contributor

geoand commented Feb 6, 2020

OK, let's merge this!

Thanks a lot for your hard work and patience @dufoli .
@loicmathieu I assume you have to rebase one of your PRs after this is in?

@geoand geoand added this to the 1.3.0 milestone Feb 6, 2020
@geoand geoand merged commit 6559064 into quarkusio:master Feb 6, 2020
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this pull request Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/documentation area/infinispan Infinispan area/mongodb area/panache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus MongoDB Client: Connect to multiple MongoDB clusters from the same app
9 participants