-
Notifications
You must be signed in to change notification settings - Fork 641
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
[nonGallery] return backfill to order by date, batch size 1000 #8510
Conversation
packages = packages | ||
.Where(p => p.Created < lastCreateTime && p.Created > startTime) | ||
.OrderBy(p => p.PackageRegistration.Id); | ||
.Where(p => p.PackageStatusKey == PackageStatus.Available || p.PackageStatusKey == PackageStatus.Validating) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validating
packages won't be available in v3-flatcontainer. Could we stick to PackageStatus.Available
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelverhagen Great insight. Thank you. Note that this is for all backfill jobs--this is the parent class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about the validating packages addition.
@joelverhagen note that this is removed code that I reintroduced. It could be an implementation detail (i.e. a flag) set in the subclasses if we like, defaulting to false. |
a96e185
to
6ef7d6c
Compare
{ | ||
supportedFrameworks = _packageService.GetSupportedFrameworks(nuspecReader, files); | ||
} | ||
catch (Exception e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can catch a more specific exception here, right? I think it's a specific one that is caused by the bad portables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch (Exception e) | |
catch |
if we end up using a generic catch-all like the other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little generic--it's an ArgumentException
which is why I opted for catching all. But I'm happy to make it explicit as exceptions not of this type will fall through and be logged, which could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to catch a more specific exception if possible. But otherwise looks good.
6ef7d6c
to
6bd2309
Compare
Backfill job was ordered in such a way as to not work with cursoring. There was a filtering condition removed which also needed to be reinstated. I also upped the batching size to 1000 for the TFM backfill.