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.

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

Jump to bottom

Resolves #647 - Fixes typos in Mongo advisory locking parameters #648

Merged
merged 8 commits into from Nov 5, 2021
Merged

Resolves #647 - Fixes typos in Mongo advisory locking parameters #648

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
18dd02b
Resolves #647 - Fixes typos in Mongo advisory locking parameters
Oct 27, 2021
b63f099
PR Feedback for #647 - Error out when both params set
Nov 4, 2021
fa52996
Fixes lint issues with #647
Nov 5, 2021
2fe1563
Additional PR Fixes #647
Nov 5, 2021
4ba4318
Lint fixes again #647
Nov 5, 2021
868d692
One more tweak to CONTRIBUTING.md
Nov 5, 2021
dd50406
One more tweak to CONTRIBUTING.md, again
Nov 5, 2021
de8708a
Merge branch 'master' into issue-647
SJrX Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions database/mongodb/README.md
View file
Open in desktop
Expand Up @@ -15,8 +15,8 @@
| `x-transaction-mode` | `TransactionMode` | If set to `true` wrap commands in [transaction](https://docs.mongodb.com/manual/core/transactions). Available only for replica set. Driver is using [strconv.ParseBool](https://golang.org/pkg/strconv/#ParseBool) for parsing|
| `x-advisory-locking` | `true` | Feature flag for advisory locking, if set to false, disable advisory locking |
| `x-advisory-lock-collection` | `migrate_advisory_lock` | The name of the collection to use for advisory locking.|
| `x-advisory-lock-timout` | `15` | The max time in seconds that the advisory lock will wait if the db is already locked. |
| `x-advisory-lock-timout-interval` | `10` | The max timeout in seconds interval that the advisory lock will wait if the db is already locked. |
| `x-advisory-lock-timeout` | `15` | The max time in seconds that migrate will wait to acquire a lock before failing. |
| `x-advisory-lock-timeout-interval` | `10` | The max time in seconds between attempts to acquire the advisory lock, the lock is attempted to be acquired using an exponential backoff algorithm. |
| `dbname` | `DatabaseName` | The name of the database to connect to |
| `user` | | The user to sign in as. Can be omitted |
| `password` | | The user's password. Can be omitted |
Expand Down
13 changes: 11 additions & 2 deletions database/mongodb/mongodb.go
View file
Open in desktop
Expand Up @@ -137,7 +137,16 @@ func (m *Mongo) Open(dsn string) (database.Driver, error) {
if err != nil {
return nil, err
}
maxLockingIntervals, err := parseInt(unknown.Get("x-advisory-lock-timout-interval"), DefaultLockTimeoutInterval)

lockTimeout := unknown.Get("x-advisory-lock-timeout-interval")

if lockTimeout == "" {
// The initial release had a typo for this argument but for backwards compatibility sake, we will keep supporting it.
lockTimeout = unknown.Get("x-advisory-lock-timout-interval")
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for preserving the old behavior! Let's return an error explaining that only one should be specified if both are specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that really important. I am on vacation for a few days, but the only way that would happen is if someone looked in the source code to see this behavior as we aren't documenting both, so I'm not sure if it is the end of the world if we just use the correct one.

Copy link
Member

Choose a reason for hiding this comment

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

This fix isn't urgent so can wait until you're back from vacation. Or I can make the change (based off of your change). I'd like to support existing users in a backwards compatible manner but also eliminate any confusion if both parameters are specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hows that @dhui ?

}

maxLockCheckInterval, err := parseInt(lockTimeout, DefaultLockTimeoutInterval)

if err != nil {
return nil, err
}
Expand All @@ -157,7 +166,7 @@ func (m *Mongo) Open(dsn string) (database.Driver, error) {
CollectionName: lockCollection,
Timeout: lockingTimout,
Enabled: advisoryLockingFlag,
Interval: maxLockingIntervals,
Interval: maxLockCheckInterval,
},
})
if err != nil {
Expand Down