Add RenameExpiredDeletedAccountsTask to release usernames of accounts that had been deleted more than a year ago #802
Conversation
Update the revalidation job's configurations: 1. `MaxPackageCreationDate` - This is the package upload cutoff after which packages will not be revalidated. The plan is to deploy repository signing this morning, so the value is now set to 8/8/2018 4:59PM PST. I won't merge this change until after we've verified the repository signing deployment. 2. `MaxPackageEventRate` - The revalidation job enqueues revalidations to try to reach this maximum package event rate. I lowered this setting to 200 package events per hour. This means that if there were 100 package pushes in the last hour, the revalidation job will enqueue revalidations at 100 revalidations per hour. PROD's package event rate has spanned from 0-300 per hour this week, so 200 package events per hour should be a safe starting point. This does mean that there will be periods where the revalidation job will sit idly due to its quota being met. This is fine, we will raise the `MaxPackageEventRate` over time as we build confidence in this job.
Improves the code that finds the information about package registrations by: 1. Sleeping between batches of registrations 2. Creating a new scope for each batch
Split up the initialization phase into multiple scopes. Each phase uses its own scope, and each batch within each phase uses its own scope (as there's a 30 second sleep time between initialization batches).
The initialization phase of the job has to insert 1.5 million records into the validation DB in batches of 1,000 records. On DEV, each batch was taking 30 - 120 seconds using Entity Framework. This fix uses `SqlBulkCopy` to speed up batches to be sub-second.
This script will be run on the first deployment of the revalidation job. It calls the initialization phase and exits. This script can be registered as a Windows service as: 1. The initialization phase verifies that the initialization phase hasn't previously completed. If it has previously completed, the initialization phase exits. 2. The initialization phase cleans up the state if the previous initialization attempt failed.
[ReleasePrep][2018.08.13]RI of dev into master
Symbol Validator Job - Update service bus configs
…ements (#534) Improve the Process Signature job for the following scenarios: 1. If a package's primary signature is a repository signature, validate that the package passes its registration's author signing certificate requirements. 2. If a package is already available, skip its registration's author signing certificate requirements. This is needed as the requirements may have changed since the package was uploaded. Addresses https://github.com/NuGet/Engineering/issues/1603 Addresses https://github.com/NuGet/Engineering/issues/1637
[ReleasePrep][2018.08.16]RI of dev into master
[ReleasePrep][2018.08.20]FI of master into dev
[ReleasePrep][2018.08.20]RI of dev into master
…tead of max lag of any search instance (#541)
Merge hardcode tables change and AI hotfix into dev
* Update instance generation to understand azureSearch type. Pipe serviceType through. * Support AzureSearchDiagResponse. Pipe ServiceType through to telemetry. * Fix tests. * Fixed committimestamp to be pessimistic. * Add test for commit time stamp. PR feedback. * Async test.
Merge branch 'master' into dev
…d reuse it in deleteexpireddeletedaccountstask
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
dtivel,
cristinamanum,
skofman1,
dannyjdev and
zhhyu
scottbommarito
changed the title
|
|
||
| _logger.LogInformation("Updated {0} entities. Expected={1}.", rowCount, expectedRowCount); | ||
|
|
||
| if (expectedRowCount != rowCount) |
There was a problem hiding this comment.
Good catch, and this problem likely exists today in DeleteExpiredVerificationKeysTask. I can wrap this in a transaction and then it should be fine.
There was a problem hiding this comment.
Had to move the call out of Dapper and use SqlCommand and SqlDataReader instead but it works great.
| var parameters = expiredKeys.Select(c => new SqlParameter("@Key" + numKeys++, SqlDbType.Int) { Value = c }).ToArray(); | ||
| command.Parameters.AddRange(parameters); | ||
|
|
||
| command.CommandText = string.Format(GetUpdateQuery(), string.Join(",", parameters.Select(p => p.ParameterName))); |
There was a problem hiding this comment.
Why not just concatenate array of ints here? We are not operating on user input here and it's quite impossible to do SQL injection through an array of ints.
There was a problem hiding this comment.
Also, since we are doing raw SQL anyway, that round trip of expired keys is quite unnecessary. Retrieve and update can be done completely on SQL server side.
There was a problem hiding this comment.
I think parameters are much cleaner, and according to the SDL, we're required to use parameters if it's possible for us to. In the past, we've only done concatenated ints when we have too many parameters (around 2-3 thousand) and SQL blocks us. I just checked PROD and found that we only have around 225 accounts that were deleted more than a year ago right now, so this isn't won't be a problem.
There was a problem hiding this comment.
It would be nice to update the scripts to make it not have to do a round-trip, but I'd rather not do the change now because
- it's nontrivial to rewrite this and the existing task in a way that removes the round-trip
- if we wanted to do logging or more verification on the accounts returned, we'd need to do it in this way
| var parameters = expiredKeys.Select(c => new SqlParameter("@Key" + numKeys++, SqlDbType.Int) { Value = c }).ToArray(); | ||
| command.Parameters.AddRange(parameters); | ||
|
|
||
| command.CommandText = string.Format(GetUpdateQuery(), string.Join(",", parameters.Select(p => p.ParameterName))); |
There was a problem hiding this comment.
Also, do you expect to run into any limits with query size by doing this? Should we split updates into batches at some point?
There was a problem hiding this comment.
The query size limit is massive...we would have to not run this job for a very long time.
|
|
||
| namespace Gallery.Maintenance | ||
| { | ||
| public abstract class UpdateEntityTask<TEntity> : MaintenanceTask |
There was a problem hiding this comment.
What do you think of making an interface for the abstract members? Then, RenameExpiredDeletedAccountsTask and DeleteExpiredVerificationKeysTask can implement that interface. I don't feel strongly about this for this change (since there's no benefit for this particular change), but the interface pattern is easier to test.
There was a problem hiding this comment.
I think this would be a nice refactor, but the job uses some janky reflection to initialize these classes, so I'd like to defer this change for now.
[Advanced Search] Add sorting & filtering to the search service (#794)
[Advanced Search] Add sorting & filtering to the search service (#794)
|
Closing. This process will remain manual for now. |
Part of https://github.com/NuGet/Engineering/issues/1303
Depends on NuGet/NuGetGallery#7471
Added
RenameExpiredDeletedAccountsTask, which renames deleted accounts older than one year and marks them as released.To do this, I refactored
DeleteExpiredVerificationKeysTaskto reuse much of its logic. The two tasks now shareUpdateEntityTask<TEntity>, which primarily handles the logic for querying the database.