[Revalidation] Speed up bulk insertions #530
Conversation
loic-sharma
requested review from
joelverhagen,
agr,
xavierdecoster,
shishirx34,
zivkan,
ryuyu,
dtivel,
chenriksson,
cristinamanum,
skofman1,
scottbommarito and
zhhyu
loic-sharma
force-pushed
the
loshar-perf
branch
from
891aa52 to
90705f3
Compare
| l.Contains("Billy") && | ||
| l.Contains("Bob"))), | ||
| // Ensure that the owners are lexicographically ordered. | ||
| l.Count() == 4 && |
There was a problem hiding this comment.
This test was out of date.
There was a problem hiding this comment.
It was failing in the CI? Or not being run?
There was a problem hiding this comment.
It's failing the CI. I have no idea how the CI didn't fail until now... I think someone may have accidentally reverted the test.
|
|
||
| var bulkCopy = new SqlBulkCopy( | ||
| _connection, | ||
| SqlBulkCopyOptions.TableLock | SqlBulkCopyOptions.FireTriggers | SqlBulkCopyOptions.UseInternalTransaction, |
There was a problem hiding this comment.
Will TableLock have any impact to the validation pipeline?
Based on your DEV testing, sounds like lock duration would be <1s? What's the frequency of locks / interval between batches?
There was a problem hiding this comment.
Looks like there's a SqlBulkCopy.BulkCopyTimeout property... maybe we should use that to ensure that bulk copies don't accidentally keep the table lock for significant time?
If the frequency of locks causes pipeline degradation, I assume we can flip the killswitch to disable?
There was a problem hiding this comment.
Will
TableLockhave any impact to the validation pipeline?
This inserts records to the PackageRevalidations table which is only used by the revalidation job, so this shouldn't impact the validation pipeline.
Based on your DEV testing, sounds like lock duration would be <1s? What's the frequency of locks / interval between batches?
There's a 30 second sleep time between batches, and there will be roughly ~1,500 batches (1.5 million packages inserted in batches of a thousand).
Looks like there's a SqlBulkCopy.BulkCopyTimeout property... maybe we should use that to ensure that bulk copies don't accidentally keep the table lock for significant time?
Good idea, I'll look into that.
If the frequency of locks causes pipeline degradation, I assume we can flip the killswitch to disable?
Yup, that's correct
There was a problem hiding this comment.
Whew - I missed that it was using a separate table from the pipeline, so this sounds great!
There was a problem hiding this comment.
The default BulkCopyTimeout value is 30 seconds, which seems reasonable for our scenario.
| bulkCopy.DestinationTableName = TableName; | ||
| bulkCopy.WriteToServer(table); | ||
|
|
||
| _connection.Close(); |
There was a problem hiding this comment.
nit: DbConnection.Close and DbConnection.Dispose are supposed to be functionally equivalent (had to look that up). Just wondering if we should explicitly add DbConnection.Dispose so that the behavior is clear from code.
There was a problem hiding this comment.
I talked to @chenriksson offline. She recommended I create a new SqlConnection every time I call AddPackageRevalidationsAsync.
|
|
||
| public PackageRevalidationInserter(SqlConnection connection, ILogger<PackageRevalidationInserter> logger) | ||
| { | ||
| _connection = connection ?? throw new ArgumentNullException(nameof(connection)); |
There was a problem hiding this comment.
This type should be IDisposable since it owns a disposable dependency.
There was a problem hiding this comment.
Alternatively, ask DI for a Func<SqlConnection> and make the invoker of the Func dispose.
There was a problem hiding this comment.
I updated some legacy jobs to pass Func<Task<SqlConnection>>, e.g.: OpenSqlConnectionAsync<StatisticsDbConfiguration>.
This ensures we can get a new access token when we create/open a new connection.
There was a problem hiding this comment.
This seems like a common pattern that many jobs will need. I addressed this by creating ISqlConnectionFactory<T>. You can ask for ISqlConnectionFactory<ValidationDbConfiguration> or ISqlConnectionFactory<IEntitiesContext> to create connections to the database of your choice.
| /// Create a SQL Connection. | ||
| /// </summary> | ||
| /// <returns>The created SQL connection.</returns> | ||
| SqlConnection Create(); |
There was a problem hiding this comment.
Should we also add CreateAsync? Note that Create is async in ServerCommon because it may require a request for the access token.
I added the non-async overload to JobBase because I think it's necessary for the DI creation of the EF contexts. Feel free to correct if I'm wrong here.
| return new DelegateSqlConnectionFactory<GalleryDbConfiguration>( | ||
| CreateSqlConnection<GalleryDbConfiguration>, | ||
| p.GetRequiredService<ILogger<DelegateSqlConnectionFactory<GalleryDbConfiguration>>>()); | ||
| }); |
There was a problem hiding this comment.
Can we move registration of ISqlConnectionFactory to JsonConfigurationJob (now in Jobs.Common), since they would be useful to legacy jobs as well?
|
|
||
| namespace NuGet.Jobs.Validation | ||
| { | ||
| public class DelegateSqlConnectionFactory<T> : ISqlConnectionFactory<T> |
There was a problem hiding this comment.
I would leave a comment that this T is a marker type. From what I can tell it only is for simplifying wire-up.
This could also be achieved by DI magic, e.g. keyed bindings.
There was a problem hiding this comment.
The comment sounds good, I'll do that.
This could indeed also be done with keyed bindings. Personally, I prefer the marker type over the keyed bindings because:
- The type that accepts the connection factory clearly indicates which database it is connecting to. This is unclear from keyed bindings.
- Setting up keyed bindings are a pain, especially if we have to do these bindings for each type that depends on
ISqlConnectionFactory. I would like to avoid them if possible 😄
There was a problem hiding this comment.
I added the comment
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| } | ||
|
|
||
| public async Task AddPackageRevalidationsAsync(IReadOnlyList<PackageRevalidation> revalidations) |
There was a problem hiding this comment.
Are all packages in memory here? Or is this method called, batch-wise?
There was a problem hiding this comment.
This method is called batch-wise, 1000 revalidations at a time. Also, this type is recreated on each batch.
loic-sharma
force-pushed
the
loshar-perf
branch
from
ccfffb1 to
1ef95cc
Compare
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.
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
SqlBulkCopyto speed up batches to be sub-second.