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

MongoDB with Panache: allow setting per collection read preference #30118

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

loicmathieu
Copy link
Contributor

Fixes #29997

@loicmathieu loicmathieu marked this pull request as ready for review January 2, 2023 11:51
@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jan 3, 2023

I have one question about this: What happens if the user enters an incorrect value?

@gsmet
Copy link
Member

gsmet commented Jan 3, 2023

My guess would be that ReadPreference.valueOf throws a somewhat actionable error. Except you won't know about the location at all. So maybe catching the exception and give some context would be nice.

@loicmathieu
Copy link
Contributor Author

@gsmet we also do this when we build the MongoClient as we can pass the default read preference from the config.

Currently, this would generate an IAE: throw new IllegalArgumentException("No match for read preference of " + name); and the JavaDoc gives the list of allow values.

I can try/catch and give a more meaningful message but this will still be an IAE

@geoand
Copy link
Contributor

geoand commented Jan 3, 2023

A more meaningful / contextual message is always better :)

@loicmathieu loicmathieu force-pushed the per-entity-read-preference branch from 3af5026 to bb402b2 Compare January 3, 2023 13:22
@loicmathieu
Copy link
Contributor Author

@gsmet @geoand I update the error message to give more explainations.

try {
collection = collection.withReadPreference(ReadPreference.valueOf(mongoEntity.readPreference()));
} catch (IllegalArgumentException iae) {
throw new IllegalArgumentException("Illegal read preference " + mongoEntity.readPreference()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably pass the original exception as the cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this is usually a good practice but here it will not give additional information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, if you have actually seen what it looks like, I trust you :)

} catch (IllegalArgumentException iae) {
throw new IllegalArgumentException("Illegal read preference " + mongoEntity.readPreference()
+ " configured in the @MongoEntity annotation of " + entityClass.getName() + "."
+ " Supported values are primary|primaryPreferred|secondary|secondaryPreferred|nearest");
Copy link
Member

Choose a reason for hiding this comment

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

Just asking: you cannot extract the values from the enum? (I haven't checked)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not en enum but a regular class.

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 3, 2023

Failing Jobs - Building bb402b2

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: integration-tests/mongodb-panache 

📦 integration-tests/mongodb-panache

io.quarkus.it.mongodb.panache.MongodbPanacheResourceTest.testMoreEntityFunctionalities line 391 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

io.quarkus.it.mongodb.panache.MongodbPanacheResourceTest.testMoreRepositoryFunctionalities line 396 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

io.quarkus.it.mongodb.panache.reactive.ReactiveMongodbPanacheResourceTest.testMoreEntityFunctionalities line 349 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

io.quarkus.it.mongodb.panache.reactive.ReactiveMongodbPanacheResourceTest.testMoreRepositoryFunctionalities line 354 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

@geoand
Copy link
Contributor

geoand commented Jan 3, 2023

Weird how the M1 tests are failing on some Mongo tests... Is it related?

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.

Let's get this in. As for the M1 failures, it's hard to know and we don't have a M1 host anymore.

@gsmet gsmet merged commit dd48b4f into quarkusio:main Jan 30, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 30, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MongoDB Panache - Read Preference on Entity level
3 participants