Skip to content

GODRIVER-3043 Use default write/read concerns in the index search commands. #1563

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

Merged
merged 4 commits into from
Apr 19, 2024
Merged

GODRIVER-3043 Use default write/read concerns in the index search commands. #1563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9dd01f
GODRIVER-3043 Use default write/read concerns in the index search com…
qingyang-hu Feb 16, 2024
9765385
Merge branch 'v1' into godriver3122
qingyang-hu Feb 16, 2024
1cfc999
minor updates
qingyang-hu Mar 21, 2024
23d2c2a
Merge branch 'v1' into godriver3122
qingyang-hu Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ evg-test-load-balancers:

.PHONY: evg-test-search-index
evg-test-search-index:
go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(TEST_TIMEOUT)s >> test.suite
# Double the timeout to wait for the responses from the server.
go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(shell echo "$$(( $(TEST_TIMEOUT) * 2))")s >> test.suite

.PHONY: evg-test-ocsp
evg-test-ocsp:
Expand Down
5 changes: 4 additions & 1 deletion mongo/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,8 +1775,11 @@ func (coll *Collection) Indexes() IndexView {

// SearchIndexes returns a SearchIndexView instance that can be used to perform operations on the search indexes for the collection.
func (coll *Collection) SearchIndexes() SearchIndexView {
c, _ := coll.Clone() // Clone() always return a nil error.
c.readConcern = nil
c.writeConcern = nil
return SearchIndexView{
coll: coll,
coll: c,
}
}

Expand Down
115 changes: 88 additions & 27 deletions mongo/integration/search_index_prose_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package integration

import (
"bytes"
"context"
"os"
"sync"
Expand All @@ -20,6 +21,8 @@ import (
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/integration/mtest"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readconcern"
"go.mongodb.org/mongo-driver/mongo/writeconcern"
)

func TestSearchIndexProse(t *testing.T) {
Expand Down Expand Up @@ -61,15 +64,16 @@ func TestSearchIndexProse(t *testing.T) {
if !cursor.Next(ctx) {
break
}
if cursor.Current.Lookup("queryable").Boolean() {
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
if name == searchName && queryable {
doc = cursor.Current
} else {
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
time.Sleep(5 * time.Second)
}
}
require.NotNil(mt, doc, "got empty document")
assert.Equal(mt, searchName, doc.Lookup("name").StringValue(), "unmatched name")
expected, err := bson.Marshal(definition)
require.NoError(mt, err, "failed to marshal definition")
actual := doc.Lookup("latestDefinition").Value
Expand Down Expand Up @@ -110,7 +114,9 @@ func TestSearchIndexProse(t *testing.T) {
if !cursor.Next(ctx) {
return nil
}
if cursor.Current.Lookup("queryable").Boolean() {
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
if name == *opts.Name && queryable {
return cursor.Current
}
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
Expand All @@ -126,7 +132,6 @@ func TestSearchIndexProse(t *testing.T) {

doc := getDocument(opts)
require.NotNil(mt, doc, "got empty document")
assert.Equal(mt, *opts.Name, doc.Lookup("name").StringValue(), "unmatched name")
expected, err := bson.Marshal(definition)
require.NoError(mt, err, "failed to marshal definition")
actual := doc.Lookup("latestDefinition").Value
Expand Down Expand Up @@ -162,15 +167,16 @@ func TestSearchIndexProse(t *testing.T) {
if !cursor.Next(ctx) {
break
}
if cursor.Current.Lookup("queryable").Boolean() {
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
if name == searchName && queryable {
doc = cursor.Current
} else {
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
time.Sleep(5 * time.Second)
}
}
require.NotNil(mt, doc, "got empty document")
require.Equal(mt, searchName, doc.Lookup("name").StringValue(), "unmatched name")

err = view.DropOne(ctx, searchName)
require.NoError(mt, err, "failed to drop index")
Expand Down Expand Up @@ -204,37 +210,49 @@ func TestSearchIndexProse(t *testing.T) {
require.NoError(mt, err, "failed to create index")
require.Equal(mt, searchName, index, "unmatched name")

getDocument := func() bson.Raw {
for {
cursor, err := view.List(ctx, opts)
require.NoError(mt, err, "failed to list")
var doc bson.Raw
for doc == nil {
cursor, err := view.List(ctx, opts)
require.NoError(mt, err, "failed to list")

if !cursor.Next(ctx) {
return nil
}
if cursor.Current.Lookup("queryable").Boolean() {
return cursor.Current
}
if !cursor.Next(ctx) {
break
}
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
if name == searchName && queryable {
doc = cursor.Current
} else {
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
time.Sleep(5 * time.Second)
}
}

doc := getDocument()
require.NotNil(mt, doc, "got empty document")
require.Equal(mt, searchName, doc.Lookup("name").StringValue(), "unmatched name")

definition = bson.D{{"mappings", bson.D{{"dynamic", true}}}}
err = view.UpdateOne(ctx, searchName, definition)
require.NoError(mt, err, "failed to drop index")
doc = getDocument()
require.NotNil(mt, doc, "got empty document")
assert.Equal(mt, searchName, doc.Lookup("name").StringValue(), "unmatched name")
assert.Equal(mt, "READY", doc.Lookup("status").StringValue(), "unexpected status")
expected, err := bson.Marshal(definition)
require.NoError(mt, err, "failed to marshal definition")
actual := doc.Lookup("latestDefinition").Value
assert.Equal(mt, expected, actual, "unmatched definition")
err = view.UpdateOne(ctx, searchName, definition)
require.NoError(mt, err, "failed to update index")
for doc == nil {
cursor, err := view.List(ctx, opts)
require.NoError(mt, err, "failed to list")

if !cursor.Next(ctx) {
break
}
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
status := cursor.Current.Lookup("status").StringValue()
latestDefinition := doc.Lookup("latestDefinition").Value
if name == searchName && queryable && status == "READY" && bytes.Equal(latestDefinition, expected) {
doc = cursor.Current
} else {
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
time.Sleep(5 * time.Second)
}
}
require.NotNil(mt, doc, "got empty document")
})

mt.Run("case 5: dropSearchIndex suppresses namespace not found errors", func(mt *mtest.T) {
Expand All @@ -250,4 +268,47 @@ func TestSearchIndexProse(t *testing.T) {
err = collection.SearchIndexes().DropOne(ctx, "foo")
require.NoError(mt, err)
})

mt.RunOpts("case 6: Driver can successfully create and list search indexes with non-default readConcern and writeConcern",
mtest.NewOptions().CollectionOptions(options.Collection().SetWriteConcern(writeconcern.New(writeconcern.W(1))).SetReadConcern(readconcern.Majority())),
func(mt *mtest.T) {
ctx := context.Background()

_, err := mt.Coll.InsertOne(ctx, bson.D{})
require.NoError(mt, err, "failed to insert")

view := mt.Coll.SearchIndexes()

definition := bson.D{{"mappings", bson.D{{"dynamic", false}}}}
const searchName = "test-search-index-case6"
opts := options.SearchIndexes().SetName(searchName)
index, err := view.CreateOne(ctx, mongo.SearchIndexModel{
Definition: definition,
Options: opts,
})
require.NoError(mt, err, "failed to create index")
require.Equal(mt, searchName, index, "unmatched name")
var doc bson.Raw
for doc == nil {
cursor, err := view.List(ctx, opts)
Copy link
Member

@prestonvasquez prestonvasquez Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not exhaust the cursor?

var doc bson.Raw
for cursor.Next(ctx) {
	name := cursor.Current.Lookup("name").StringValue()
	queryable := cursor.Current.Lookup("queryable").Boolean()
	if name == searchName && queryable {
		doc = cursor.Current
		break
	}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I break on negative cursor.Next to reduce the deep nesting block and simplify the exiting of the recurring List calls.

Copy link
Member

Choose a reason for hiding this comment

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

I mention doing this because this is what CXX does: https://github.com/kevinAlbs/mongo-cxx-driver/blob/fff7c2c71438f9f62c6b1a6ca5dee20f1c1749ac/src/mongocxx/test/search_index_view.cpp#L19

I think the current implementation is fine, though. IIUC in case 6 there would only ever be one doc on the cursor for view.List. Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct.

require.NoError(mt, err, "failed to list")

if !cursor.Next(ctx) {
break
}
name := cursor.Current.Lookup("name").StringValue()
queryable := cursor.Current.Lookup("queryable").Boolean()
if name == searchName && queryable {
doc = cursor.Current
} else {
t.Logf("cursor: %s, sleep 5 seconds...", cursor.Current.String())
time.Sleep(5 * time.Second)
}
}
require.NotNil(mt, doc, "got empty document")
expected, err := bson.Marshal(definition)
require.NoError(mt, err, "failed to marshal definition")
actual := doc.Lookup("latestDefinition").Value
assert.Equal(mt, expected, actual, "unmatched definition")
})
}
40 changes: 8 additions & 32 deletions mongo/search_index_view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/writeconcern"
"go.mongodb.org/mongo-driver/x/bsonx/bsoncore"
"go.mongodb.org/mongo-driver/x/mongo/driver"
"go.mongodb.org/mongo-driver/x/mongo/driver/operation"
Expand Down Expand Up @@ -134,20 +133,13 @@ func (siv SearchIndexView) CreateMany(
return nil, err
}

wc := siv.coll.writeConcern
if sess.TransactionRunning() {
wc = nil
}
if !writeconcern.AckWrite(wc) {
sess = nil
}

selector := makePinnedSelector(sess, siv.coll.writeSelector)

op := operation.NewCreateSearchIndexes(indexes).
Session(sess).WriteConcern(wc).ClusterClock(siv.coll.client.clock).
Database(siv.coll.db.name).Collection(siv.coll.name).CommandMonitor(siv.coll.client.monitor).
Deployment(siv.coll.client.deployment).ServerSelector(selector).ServerAPI(siv.coll.client.serverAPI).
Session(sess).CommandMonitor(siv.coll.client.monitor).
ServerSelector(selector).ClusterClock(siv.coll.client.clock).
Collection(siv.coll.name).Database(siv.coll.db.name).
Deployment(siv.coll.client.deployment).ServerAPI(siv.coll.client.serverAPI).
Timeout(siv.coll.client.timeout)

err = op.Execute(ctx)
Expand Down Expand Up @@ -196,20 +188,12 @@ func (siv SearchIndexView) DropOne(
return err
}

wc := siv.coll.writeConcern
if sess.TransactionRunning() {
wc = nil
}
if !writeconcern.AckWrite(wc) {
sess = nil
}

selector := makePinnedSelector(sess, siv.coll.writeSelector)

op := operation.NewDropSearchIndex(name).
Session(sess).WriteConcern(wc).CommandMonitor(siv.coll.client.monitor).
Session(sess).CommandMonitor(siv.coll.client.monitor).
ServerSelector(selector).ClusterClock(siv.coll.client.clock).
Database(siv.coll.db.name).Collection(siv.coll.name).
Collection(siv.coll.name).Database(siv.coll.db.name).
Deployment(siv.coll.client.deployment).ServerAPI(siv.coll.client.serverAPI).
Timeout(siv.coll.client.timeout)

Expand Down Expand Up @@ -258,20 +242,12 @@ func (siv SearchIndexView) UpdateOne(
return err
}

wc := siv.coll.writeConcern
if sess.TransactionRunning() {
wc = nil
}
if !writeconcern.AckWrite(wc) {
sess = nil
}

selector := makePinnedSelector(sess, siv.coll.writeSelector)

op := operation.NewUpdateSearchIndex(name, indexDefinition).
Session(sess).WriteConcern(wc).CommandMonitor(siv.coll.client.monitor).
Session(sess).CommandMonitor(siv.coll.client.monitor).
ServerSelector(selector).ClusterClock(siv.coll.client.clock).
Database(siv.coll.db.name).Collection(siv.coll.name).
Collection(siv.coll.name).Database(siv.coll.db.name).
Deployment(siv.coll.client.deployment).ServerAPI(siv.coll.client.serverAPI).
Timeout(siv.coll.client.timeout)

Expand Down
2 changes: 1 addition & 1 deletion testdata/index-management/searchIndexIgnoresReadWriteConcern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
]
},
{
"description": "createSearchIndex ignores read and write concern",
"description": "createSearchIndexes ignores read and write concern",
"operations": [
{
"name": "createSearchIndexes",
Expand Down
4 changes: 2 additions & 2 deletions testdata/index-management/searchIndexIgnoresReadWriteConcern.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ tests:
writeConcern: { $$exists: false }
readConcern: { $$exists: false }

- description: "createSearchIndex ignores read and write concern"
- description: "createSearchIndexes ignores read and write concern"
operations:
- name: createSearchIndexes
object: *collection0
arguments:
arguments:
models: []
expectError:
# This test always errors in a non-Atlas environment. The test functions as a unit test by asserting
Expand Down
42 changes: 18 additions & 24 deletions x/mongo/driver/operation/create_search_indexes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,25 @@ import (
"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/event"
"go.mongodb.org/mongo-driver/mongo/description"
"go.mongodb.org/mongo-driver/mongo/writeconcern"
"go.mongodb.org/mongo-driver/x/bsonx/bsoncore"
"go.mongodb.org/mongo-driver/x/mongo/driver"
"go.mongodb.org/mongo-driver/x/mongo/driver/session"
)

// CreateSearchIndexes performs a createSearchIndexes operation.
type CreateSearchIndexes struct {
indexes bsoncore.Document
session *session.Client
clock *session.ClusterClock
collection string
monitor *event.CommandMonitor
crypt driver.Crypt
database string
deployment driver.Deployment
selector description.ServerSelector
writeConcern *writeconcern.WriteConcern
result CreateSearchIndexesResult
serverAPI *driver.ServerAPIOptions
timeout *time.Duration
indexes bsoncore.Document
session *session.Client
clock *session.ClusterClock
collection string
monitor *event.CommandMonitor
crypt driver.Crypt
database string
deployment driver.Deployment
selector description.ServerSelector
result CreateSearchIndexesResult
serverAPI *driver.ServerAPIOptions
timeout *time.Duration
}

// CreateSearchIndexResult represents a single search index result in CreateSearchIndexesResult.
Expand Down Expand Up @@ -109,9 +107,15 @@ func (csi *CreateSearchIndexes) Execute(ctx context.Context) error {
return driver.Operation{
CommandFn: csi.command,
ProcessResponseFn: csi.processResponse,
Client: csi.session,
Clock: csi.clock,
CommandMonitor: csi.monitor,
Crypt: csi.crypt,
Database: csi.database,
Deployment: csi.deployment,
Selector: csi.selector,
ServerAPI: csi.serverAPI,
Timeout: csi.timeout,
Copy link
Member

Choose a reason for hiding this comment

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

It's peculiar all this data was missing from the operation. Was it just missed in the original implementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it will be better to align with the implementation in "create_indexes.go".

Copy link
Collaborator

@matthewdale matthewdale Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, the search index management server commands actually manage resources on an Apache Lucene cluster (and maybe some database configuration), so it's possible some of the previously omitted info was unnecessary. However, it's not clear what the impact of adding or removing those fields is.

I generally agree that passing the same info as CreateIndexes seems like a good idea, but should we test for that? Or if the info was truly unnecessary, should we continue to omit it?

Edit: To clarify, we shouldn't block merging the PR, but we should try to figure out whether we're missing test coverage for this type of behavior change that could be hiding other omissions.

}.Execute(ctx)

}
Expand Down Expand Up @@ -214,16 +218,6 @@ func (csi *CreateSearchIndexes) ServerSelector(selector description.ServerSelect
return csi
}

// WriteConcern sets the write concern for this operation.
func (csi *CreateSearchIndexes) WriteConcern(writeConcern *writeconcern.WriteConcern) *CreateSearchIndexes {
if csi == nil {
csi = new(CreateSearchIndexes)
}

csi.writeConcern = writeConcern
return csi
}

// ServerAPI sets the server API version for this operation.
func (csi *CreateSearchIndexes) ServerAPI(serverAPI *driver.ServerAPIOptions) *CreateSearchIndexes {
if csi == nil {
Expand Down
Loading