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

Automatically Create Database if Not Present #49

Merged
merged 6 commits into from Oct 28, 2021
Merged

Automatically Create Database if Not Present #49

merged 6 commits into from Oct 28, 2021

Conversation

wsugarman
Copy link
Member

As per #48, this change allows the worker to create the database automatically if it doesn't exist. Because the worker must gracefully handle the possibility the DB hasn't been created, a few changes needed to be made:

  • The initial connection to the SQL server must use another Initial Catalog or Database instead of the user-specified one. As such, the worker now initially connects to the system database master
  • The lock for upgrading the schema was also previously obtained within the context of the user-specified Database, so now the lock is only obtained after the presence of the database has been verified.
    • Each worker now independently checks the existence of the database.
  • New log events have been created for executing arbitrary SQL commands/statements and creating a database
  • New log assertion APIs have been added to accommodate non-deterministic scenarios, such as the initial inspection of whether the database exists

async Task CreateDatabaseAsync(string databaseName, SqlConnection connection)
{
using SqlCommand command = connection.CreateCommand();
command.CommandText = $"CREATE DATABASE {Identifier.Escape(databaseName)} COLLATE Latin1_General_100_BIN2_UTF8";
Copy link
Member Author

Choose a reason for hiding this comment

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

We use Identifier.Escape(string) to handle possibly non-standard names as well as protect against any malicious queries while we're setting up.

src/DurableTask.SqlServer/SqlDbManager.cs Show resolved Hide resolved
LogAssert.ExecutedSqlScript("permissions.sql"),
LogAssert.SprocCompleted("dt._UpdateVersion"));
LogAssert
.For(this.logProvider)
Copy link
Member Author

Choose a reason for hiding this comment

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

This refactor may be a bit much.

I wanted a "simple" way to convey that some logs were expected in any order, some were expected in a certain order, and some where condition -- all for the same execution!

Copy link
Member

Choose a reason for hiding this comment

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

I like this design! I was thinking it would be necessary to have more flexible log validation at some point, and it looks like you've added that for me, which I appreciate. :)

Copy link
Member Author

@wsugarman wsugarman Oct 28, 2021

Choose a reason for hiding this comment

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

Thanks! This was a compromise in my head over creating some sort of LINQ-like decorators (e.g. the functions Contains or Sequence could return an interface ILogAssertion).

LogAssert.ExecutedSqlScript("permissions.sql"),
LogAssert.SprocCompleted("dt._UpdateVersion"),
// 2nd
LogAssert.AcquiredAppLock(),
Copy link
Member Author

Choose a reason for hiding this comment

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

For the other workers, I don't think we can guarantee their response codes other than success; there may or may not be contention for the lock.

global.json Outdated Show resolved Hide resolved
@cgillum cgillum self-assigned this Oct 26, 2021
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

This is an excellent PR and I really appreciate the work and care that went into it. I just have a few minor feedback items and then I'd love to get this merged.

src/DurableTask.SqlServer/Identifier.cs Outdated Show resolved Hide resolved
src/DurableTask.SqlServer/Identifier.cs Outdated Show resolved Hide resolved
src/DurableTask.SqlServer/SqlDbManager.cs Show resolved Hide resolved
LogAssert.ExecutedSqlScript("permissions.sql"),
LogAssert.SprocCompleted("dt._UpdateVersion"));
LogAssert
.For(this.logProvider)
Copy link
Member

Choose a reason for hiding this comment

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

I like this design! I was thinking it would be necessary to have more flexible log validation at some point, and it looks like you've added that for me, which I appreciate. :)

test.runsettings Outdated Show resolved Hide resolved
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this contribution! It will definitely reduce the setup burden for many users, and the new test functionality will be helpful as well.

@cgillum cgillum merged commit 215befc into microsoft:main Oct 28, 2021
@wsugarman
Copy link
Member Author

No problem! Happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants