Remove write and read concern from Atlas Search Index commands. #1241
Conversation
|
Evergreen task: test-atlas-search-index-helpers |
...sync/src/test/functional/com/mongodb/client/AbstractAtlasSearchIndexManagementProseTest.java
Outdated
Show resolved
Hide resolved
JAVA-5233
JAVA-5233
JAVA-5233
There was a problem hiding this comment.
The task test-atlas-search-index-helpers fails with
AtlasSearchIndexManagementProseTest > Case 1: Driver can successfully create and list search indexes FAILED
[2023/11/03 21:24:40.234] com.mongodb.MongoCommandException: Command failed with error 72 (InvalidOptions): 'Command aggregate does not support { readConcern: { level: "majority" ...
Weirdly, this test failure is reported as SYSTEM FAILED.
… Search Index commands. JAVA-5233
JAVA-5233
JAVA-5233
Fixed. Evergreen task: test-atlas-search-index-helpers |
vbabanin
requested review from
jyemin and
stIncMale
and removed request for
stIncMale
|
Added a new change to use the default read concern for Atlas Search Commands, which means it won't be added to commands. Would you mind giving it another look, @jyemin? |
| @@ -694,7 +694,9 @@ public ListSearchIndexesPublisher<Document> listSearchIndexes() { | |||
| @Override | |||
| public <TResult> ListSearchIndexesPublisher<TResult> listSearchIndexes(final Class<TResult> resultClass) { | |||
| notNull("resultClass", resultClass); | |||
| return new ListSearchIndexesPublisherImpl<>(mongoOperationPublisher.withDocumentClass(resultClass)); | |||
|
|
|||
| return new ListSearchIndexesPublisherImpl<>(mongoOperationPublisher | |||
There was a problem hiding this comment.
I'm not clear on why, in driver-sync's MongoCollectionImpl, read concern seems to have been removed, while here it seems like we're adding it. Can you clarify the intent?
JAVA-5233
vbabanin
changed the title
JAVA-5233
| the server will return an error. */ | ||
| if (isSearchIndexCommand(event)) { | ||
| BsonDocument command = event.getCommand(); | ||
| assertFalse(command.containsKey("writeConcern")); |
There was a problem hiding this comment.
I realized that I had noticed this earlier and then neglected to comment on it: asserting like this in any event listener is likely a no-op since the driver (I think) swallows exceptions thrown by listeners and in any case it may throw on a different thread for reactive tests.
You need to instead set some state in the listener and then assert on that state in the main test code. There are examples of this in many places in the driver.
Since this PR is already merged, you'll have to address it in a follow-up PR.
There was a problem hiding this comment.
(This is a good example of where writing a failing test first, TDD style, can catch errors in the test itself)
JAVA-5233