-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fixes #835: ensure "createCollection()" still idempotent with "no-index" options #836
Fixes #835: ensure "createCollection()" still idempotent with "no-index" options #836
Conversation
|
||
// Need to override to prevent comparison of the supplier | ||
@Override | ||
public boolean equals(Object o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "real fix".
return executeCollectionCreation(queryExecutor); | ||
} | ||
// if table exists we have to choices: | ||
// (1) trying to create with same options -> ok, proceed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proceed, but essentially nothing changes.
Am i understanding it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yuqi-Du In the sense that this is what used to happen before changes too, yes.
But I think it will now try to re-create Collection, which will be no-op due to "IF NOT EXISTS" used in CQL.
Except! Important case is that if we failed to create one or more of SAIs, these will now be created, which is why retry of a timed out creation could fix things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (o instanceof IndexingConfig other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but do we also need to check vector config here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is within nested class, IndeingConfig
and not for CollectionSettings
-- CollectionSettings
uses default, record-provided field-by-field equality check. So that will cover vectorEnabled
and other fields.
What this PR does:
Adds tests to verify idempotency of "createCollection" command when using new "no-index" options; and fixes issues found, if any
Which issue(s) this PR fixes:
Fixes #835
Checklist