PostgreSql membership tests #2113
Conversation
|
Hi @xclayl, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@xclayl, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
|
||
|
|
||
| -- For each deployment, there will be only one (active) membership version table version column which will be updated periodically. | ||
| CREATE TABLE IF NOT EXISTS OrleansMembershipVersionTable |
There was a problem hiding this comment.
This might be OK while scripting by yourself, but it might be easier to take the script and add it to some tooling (Red Gate etc.), if it were just a simple CREATE TABLE. My impression, but I don't have wide experience with all kinds of tooling.
There was a problem hiding this comment.
It's been convenient while running/debugging. If there is a change to a query, I can run the whole script and it doesn't error.
I'd be interesting in knowing what the plan is when SQL changes are needed (added SiloName column for instance). How do we help users upgrade their database?
There was a problem hiding this comment.
I've wondered if I could update the script to drop the tables. That way people could run the script to update their database. How would a running cluster of Orleans silos react if all the data in the tables were wiped out?
There was a problem hiding this comment.
I'd be interesting in knowing what the plan is when SQL changes are needed (added SiloName column for instance). How do we help users upgrade their database?
The changes are usually backwards compatible. When we have to make changes to the schema, which is very rare, we note it in the release notes. Thus far, we haven't made upgrade scripts.
I've wondered if I could update the script to drop the tables. That way people could run the script to update their database. How would a running cluster of Orleans silos react if all the data in the tables were wiped out?
I suggest you keep the semantics of the other scripts. Orleans membership will fail in case the table is wiped. Also, I wouldn't want people to mistakenly lose all their reminders data.
|
@xclayl Looks like you're in good swing. I basically scrolled it through in minute, so this isn't that exhaustive. One thing you don't know, we tried to align the SQL Server and MySQL scripts closer together so they'd be easier to compare when making changes and fixes, with regard to this importan PR, it'd mean filling the query table what looks like to me like in MySQL. |
|
If you hit a roadblock and run out of minutes in the day, don't hesitate to ping on Gitter someone or here me or @shayhatsor. |
|
@xclayl Also, the stats systems should be overhauled completely to provide numeric values and decompound the compound values. It's on my mental todo list, but I'm thin on time and I've waited for the telemetry story to finish. In case you have time and ideas, don't keep them buried but pop by on Gitter or open an issue (or maybe I should open an issue about it). |
|
@veikkoeeva thanks for the quick response. I'm happy to help with PostgreSQL changes related to the stats, but I would be unlikely to spearhead that change. There's a project I want to work on that uses Orleans, and that is my main focus.
I'm not sure what you mean here. I suspect you're saying that the pgsql code shouldn't use functions, and I'd like that too, however I don't know a way to do all that was needed (transactions, SELECTing 1 or 0, etc) in pgsql without functions. It'd make me happy for someone to prove me wrong and rewrite it without functions. I'm always happy to learn new things. |
| options.ClusterConfiguration.Globals.AdoInvariant = AdoNetInvariants.InvariantNamePostgreSql; | ||
| options.ClusterConfiguration.PrimaryNode = null; | ||
| options.ClusterConfiguration.Globals.SeedNodes.Clear(); | ||
| options.ClientConfiguration.AdoInvariant = AdoNetInvariants.InvariantNamePostgreSql; |
There was a problem hiding this comment.
you can now remove this line, since I fixed the TestClusterOptions in #2116
There was a problem hiding this comment.
@shayhatsor thanks for that -- done
Not at all, do use functions if it makes sense. Even the SQL Serve one could benefit of it if the queries would be compiled to native queries. Though that's not something everyone is willing to pay for in licences. :) Anyway, it looked to me they were spread out differently than in the MySQL case, so I wanted to just point out there's some value having them more aligned. It's easier to make changes and check them side-by-side then. I could be wrong with this, ignore in that case. It's great you're doing this. It's naturally entirely OK not to break youreself labouring here. It's good sometimes to air out ideas so that as many people as possible know potential plans. :) |
|
@veikkoeeva I want to box this off and move on to the next part (of my project) -- on to Angular2, TypeScript and Material Design. I'll be around though to support Orleans/PostgreSql. Are you able to contact me via GitHub? That last piece for Orleans/PostgreSql is the storage framework, which I don't plan to use. I get nervous (that's not quite the right word) about coding something I don't have a vested interest in, but let's box off PostgreSql. How does the process work now on? Should I create a new pull request? It looks like this pull request is tracking my master branch, cool. I'm new to git and GitHub, so please let me know if you want something done differently. |
|
@veikkoeeva After reviewing the PostgreSql and MySql scripts, the "INSERT INTO OrleansQuery" statements are in the same order. However, the functions are created in different places. The difference is the MySql script creates all the functions after "InsertMembershipKey". The PostgreSql script creates each function right before the related "INSERT INTO OrleansQuery" statement. PostgreSql script seems more organised to me, but I'm happy to follow MySql if that is preferred. |
|
@xclayl I think it's OK to leave the storage stuff out. Maybe we should split it to a separate script too. Yeah, have a pull request and people pour over it. Probably me and @shayhatsor and some of the core team members. If the tests pass, it's a good sign. I don't know your contact details and I don't know if you see mine, but if by contacting you mean pinging in some way, I get a message every time you refer to me somewhere. And I'm on plenty of channels too in Gitter, in Orleans for instance too. It's just right now I'm sluggish as we have a big push to production at work and I have other non-work relateted commitments. Prioritizing etc. :) |
|
@xclayl & @veikkoeeva , do you consider this work ready for merge? |
|
@shayhatsor The work isn't done quite yet. I'd like to tidy up some stray code and add documenation so that users know how to setup the Npgsql provider. |
|
@shayhatsor The work here is done. I see the documentation in another branch, so that will have to be another PR. |
| @@ -192,14 +192,18 @@ internal static ReminderEntry GetReminderEntry(IDataRecord record) | |||
| /// </summary> | |||
| private static string TryGetSiloName(IDataRecord record) | |||
| { | |||
| for (var i = 0; i < record.FieldCount; ++i) | |||
There was a problem hiding this comment.
I'm not sure this change is necessary, I'd rather we don't use an exception
There was a problem hiding this comment.
It was erroring since the method was looking for the column in case sensitive way. I changed it to use GetOrdinal() so that SiloName worked the same way as the other columns which use GetOrdinal() under the covers. GetOrdinal() first does a case-sensitive search and if the column isn't found, it does a case-insensitive search. One option is to duplicate this behavior so that it doesn't throw exceptions.
Another option is to return the columns with the upper-lower case originally expected. But just out of curiosity, why does this column work differently than the others? ie "TryGet..."
There was a problem hiding this comment.
I see. I personally have no problem with it looking for the column in case sensitive way. But I'm OK with your change. This column is different because we've (re)added it and wanted to keep the code backwards compatible.
There was a problem hiding this comment.
@xclayl In addition, there's always room for improvements. If you notice something and aren't in a hurry, just a make a mental note and drop it to a chat or open an issue and someone and eventually things improve. :)
I think I have have some time tomorrow to take a deeper look at this, but I need to upgrade my VM with PostregreSQL so realistically I drop comments, if there's any – looks good at a glance – on Wednesday or Thursday. If there's hurry to get into the 1.3.0 time window I can go to sleep a bit later. :) @jdom? @sergeybykov? Still time?
There was a problem hiding this comment.
@xclayl A tangential that might make sense (but can also be refactored in another PR if it makes sense)
Another option is to return the columns with the upper-lower case originally expected. But just out of curiosity, why does this column work differently than the others? ie "TryGet..."
/// <summary>
/// Returns a value if it is not <see cref="System.DBNull"/>, <em>default(TValue)</em> otherwise.
/// </summary>
/// <typeparam name="TValue">The type of the value to request.</typeparam>
/// <param name="record">The record from which to retrieve the value.</param>
/// <param name="fieldName">The name of the field to retrieve.</param>
/// <param name="default">The default value if value in position is <see cref="System.DBNull"/>.</param>
/// <returns>Either the given value or the default for the requested type.</returns>
/// <exception cref="IndexOutOfRangeException"/>
/// <remarks>This function throws if the given <see paramref="fieldName"/> does not exist.</remarks>
public static TValue GetValueOrDefault<TValue>(this IDataRecord record, string fieldName, TValue @default = default(TValue))
{
var ordinal = record.GetOrdinal(fieldName);
return record.IsDBNull(ordinal) ? @default : (TValue)record.GetValue(ordinal);
}
https://github.com/dotnet/orleans/blob/master/src/OrleansSQLUtils/Storage/DbExtensions.cs#L117
Or
`/// <summary>
/// Returns a value with the given <see paramref="fieldName"/>.
/// </summary>
/// <typeparam name="TValue">The type of value to retrieve.</typeparam>
/// <param name="record">The record from which to retrieve the value.</param>
/// <param name="fieldName">The name of the field.</param>
/// <returns>Value in the given field indicated by <see paramref="fieldName"/>.</returns>
/// <exception cref="IndexOutOfRangeException"/>
/// <remarks>This function throws if the given <see paramref="fieldName"/> does not exist.</remarks>
public static TValue GetValue<TValue>(this IDataRecord record, string fieldName)
{
var ordinal = record.GetOrdinal(fieldName);
return (TValue)record.GetValue(ordinal);
}
https://github.com/dotnet/orleans/blob/master/src/OrleansSQLUtils/Storage/DbExtensions.cs#L180
There was a problem hiding this comment.
These won't work, b/c the column may not be SELECTed at all.
|
@xclayl I'll take a look in the coming days. A first note is that you're still checking the existence of objects before creating them. The other two scripts don't do that. I suggest we do as currently in the othet two and if warranted, open an issue for discussion if the scripts should fail in case objects already exist or carry on execution. /cc @shayhatsor |
|
@veikkoeeva There isn't much difference between the two (both types of scripts expect the database to be empty), so I'm not bothered about taking out the "if exists". I might be interested in a discussion about upgrading databases, but what I've done isn't the whole answer, so I'll change the script to be more like the others. |
|
@veikkoeeva Script cleaned up. I'd like to do what I can to include this in 1.3. The only outstanding issue I know of is documentation, but that would be in a different PR. |
|
@xclayl Check! I wrote in Gitter the the tests are green, though I had slight trouble https://gitter.im/dotnet/orleans?at=57d9bfebc3e7045a304d8ebe. I'll try to follow tomorrow and see if it's a wrap as far as I can think of anything. :) |
There was a problem hiding this comment.
This is easty to overlook, remember also to update the Nuget package on include the PostgreSql script. This way it's delivered together with the binaries and it can be retrieved from the Nuget directory.
| new StorageConnection | ||
| { | ||
| StorageInvariant = AdoNetInvariants.InvariantNamePostgreSql, | ||
| ConnectionString = "Server=127.0.0.1;Port=5432;Database=postgres;Integrated Security=true;Pooling=false;" |
There was a problem hiding this comment.
@xclayl It looks like safest to remove the pooling feature. As you rightfuly pointed out, including this (without pgbouncer etc.) gives easily exceptions.
| { | ||
| setupScript, | ||
| CreateStreamTestTable | ||
| }; // setupScript.Split(new[] { "GO" }, StringSplitOptions.RemoveEmptyEntries).ToList(); |
There was a problem hiding this comment.
These extraneous comments could be removed.
There was a problem hiding this comment.
Thanks - I want this tidy too.
|
|
||
| public override string DefaultConnectionString | ||
| { | ||
| get { return @"Server=127.0.0.1;Port=5432;Database=postgres;Integrated Security=true;Pooling=false;"; } |
There was a problem hiding this comment.
@xclayl It looks like safest to remove the pooling feature. As you rightfuly pointed out, including this (without pgbouncer etc.) gives easily exceptions.
There was a problem hiding this comment.
@veikkoeeva, I believe I have this set to the most reliable way now -- I kept pooling off and restored the code that kills sessions when dropping the database.
| WHERE | ||
| ServiceId = @ServiceId AND @ServiceId IS NOT NULL; | ||
| '); | ||
|
|
There was a problem hiding this comment.
empty lines removed at the end of file (WHERE clause not changed). I think that's what you mean.
|
|
||
|
|
||
| CREATE FUNCTION update_i_am_alive_time( | ||
| deployment_id OrleansMembershipTable.DeploymentId%TYPE, |
| PeriodArg, | ||
| GrainHashArg, | ||
| 0 | ||
| ON CONFLICT (ServiceId, GrainId, ReminderName) |
There was a problem hiding this comment.
Ah, the new PostgreSQL "UPSERT" syntax, I assume (I'm a PostgreSQL n00b). 👍
There was a problem hiding this comment.
Yes -- the upsert statement is highly consistent too -- two statements running at the same time will never error -- unlike TSQL's Merge statement. But then again, TSQL's Merge has more features.
|
|
||
| CONSTRAINT PK_OrleansMembershipVersionTable_DeploymentId PRIMARY KEY(DeploymentId) | ||
| ); | ||
|
|
There was a problem hiding this comment.
This is a minor nit that doesn't extract value from this contribution, but the empty lines between the constructs vary. They could be normalized (two empty lines?).
There was a problem hiding this comment.
I'm not sure what is the issue here. The empty lines around the CONSTRAINTs looks the same throughout the file and the same to SQL Server's script. I'm pretty sure I didn't touch these lines since they match TSQL.
There was a problem hiding this comment.
Oh, I think you mean between the statements. I'll tidy it up...
|
@xclayl Easy to overlook, remember to update the Nuget file to include the PostgreSQL script. This way it can be retrieved from the Nuget folder along with the binaries. |
|
@shayhatsor Do you see anything? Otherwise than the minor comments this looks good to me! 👍 |
|
LGTM, after @xclayl makes the changes @veikkoeeva has mentioned, I'll merge. |
|
@veikkoeeva, nuget file updated. I tried to test it by building it, but I couldn't figure it out. If you know how, can you test it? |
|
@xclayl , to test the build locally, including nuget package generation, run build.cmd |
|
@shayhatsor thanks I'll test it out :) |
|
built nuget package and I can see the sql script inside the nuget package. @shayhatsor & @veikkoeeva, I believe I have done all the outstanding issues. Let me know if I've left anything off or not done it in a way you meant. Cheers, |
|
@xclayl, thank you for the contribution! |
|
@xclayl, this is great! Thank you! |
|
I want to thank you all in particular for being so welcoming to contributions. |
I don't expect this to be accepted yet -- personally, I'd like to get the Reminders and Stats tests working first.
This pull request is to give people heads-up that I'm working in this area and to get feedback on the work so far.
Cheers,
Clay
Update 2016-09-12 This PR is ready to be merged. It includes Reminders and Stats now (as well as Membership). It does not include blob storage. And it does not include changes to the documentation.