search

michaelklishin / quartz-mongodb

A MongoDB-based store for the Quartz scheduler. This fork strives to be as feature complete as possible. Originally by MuleSoft.
Other
248 stars 203 forks source link

Make MongoConnector interface #133

Closed dorogush closed 7 years ago

dorogush commented 7 years ago

Reworked the way how quartz-mongodb accesses mongodb. It is now through the MongoConnector interface. Support two implementations out of the box: the one that owns the lifecycle of MongoClient (creates and closes) and another that just uses what user provided.

dorogush commented 7 years ago

Motivation for this whole change: Currently there are two ways of interaction with Mongo:

  1. Create MongoClient ourselves using user-provided parameters. Close the client on shutdown (we own the lifecycle).
  2. Use the user-provided MongoClient instance through MongoDBJobStore constructor (we don't own the lifecycle).

Issues:

  1. In case we don't own the MongoClient, we still attempt to close it currently which is wrong.
  2. In case the MongoClient provided is configured with incorrect global WriteConcern (for example, Unacknowledged), the whole library may fail unpredictable.
dorogush commented 7 years ago

@michaelklishin please review

pwojnowski commented 7 years ago

I'm fine with this change. @michaelklishin if you have nothing against then we can merge it.

michaelklishin commented 7 years ago

I don't have a strong opinion on this but supporting user-provided "unmanaged" MongoDB connections sounds reasonable.

michaelklishin commented 7 years ago

@dorogush thank you!

michaelklishin commented 7 years ago

Curiously there is 1 failing test on CI and only on JDK 8. @dorogush can you please take a look?

dorogush commented 7 years ago

@michaelklishin @pwojnowski unfortunately, I have no idea how it may be related. Looks like unstable integration test. My guess is that it depends on the tests invocation order, and the latter is undefined?

pwojnowski commented 7 years ago

I also couldn't reproduce it locally on Java 8. I'll look at it.

dorogush commented 7 years ago

@pwojnowski any news? Can we release a new version?

michaelklishin commented 7 years ago

@dorogush we can but I'd prefer to do that after we fix CI.

pwojnowski commented 7 years ago

Sorry for the delay. My commits fixed the build, but not the issue that was in the failed build. I added creation of indexes for DAOs tests, because they relay on them. This worked only because the tests were executed either on existing test db or after integration tests, which create the indexes too.

The issue within the failing build looks like timing problem within the test itself. I couldn't reproduce it locally, but IMHO we should improve logs in test, because the current setup doesn't help.