-
Notifications
You must be signed in to change notification settings - Fork 186
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
Backfill targetted #1649
Backfill targetted #1649
Conversation
src/models/reposyncstate.ts
Outdated
@@ -37,6 +38,7 @@ export class RepoSyncState extends Model { | |||
|
|||
get status(): TaskStatus { | |||
const statuses = [this.pullStatus, this.branchStatus, this.commitStatus]; | |||
console.log(statuses); |
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.
oh the horror!
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.
What have i done! 😢
src/sync/sync-utils.ts
Outdated
fullSyncStartTime = new Date().toISOString(); | ||
if (targetTasks && targetTasks.length > 0) { |
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.
if (targetTasks?.length)
src/sync/sync-utils.ts
Outdated
} else { | ||
await subscription.update({ | ||
totalNumberOfRepos: null, | ||
repositoryCursor: null, | ||
repositoryStatus: null | ||
}); | ||
// Remove all state as we're starting anew | ||
await RepoSyncState.deleteFromSubscription(subscription); | ||
} |
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.
This logic isn't correct. It's not just for a full backfill, but we want the ability to reset specific things for partial as well.
Hm... maybe we want to stop using the concept of 'full' or 'partial' and just make it "what do you want to reset before I send this backfill message to the queue"..
…hub-for-jira into backfill-targetted
@@ -0,0 +1,54 @@ | |||
import { Request, Response } from "express"; |
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.
No change in here, just lifted from sync-utils and dumped
src/sync/sync-utils.ts
Outdated
const updateTasks = {}; | ||
targetTasks.forEach(task => { | ||
if (syncType === "full") { | ||
updateTasks[`${task}Cursor`] = null; | ||
} | ||
updateTasks[`${task}Status`] = null; | ||
}); | ||
|
||
return RepoSyncState.update({ | ||
repoUpdatedAt: null, | ||
...updateTasks | ||
}, { | ||
where: { | ||
subscriptionId: subscription.id | ||
} | ||
}); |
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.
This completely forgets the repository
task that's on subscription model (not reposyncstate), but also, it will also try to update repositoryStatus
which doesn't exist on RepoSyncState and I think will error?
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.
ohhh ok, i thought it was on the reposyncstate, but that make sense since its a higher level. copy that.
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.
I don't think we need to split this up into extra functions, especially since they have duplicate code in it. just a simple if statement for repository
task should do
src/sync/sync-utils.ts
Outdated
return; | ||
} | ||
|
||
const subscriptionTasks = targetTasks.find(task => task === "repository"); |
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.
don't you mean filter
? or I guess you could change it to hasRepositoryTask = targetTasks.includes("repository");
src/sync/sync-utils.ts
Outdated
@@ -49,6 +54,57 @@ export async function findOrStartSync( | |||
}, 0, logger); | |||
} | |||
|
|||
type subscriptionUpdateTasks = { |
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.
Types should always start capitalized
src/sync/sync-utils.ts
Outdated
updateSubscriptionTasks.repositoryStatus = null; | ||
} | ||
|
||
await subscription.update({ |
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.
This should only be called in the if statement above. You don't need to call this when there's nothing to update.
You also don't need to use the spread operator when it's the only key in the object, just use updateSubscriptionTasks
as the update object.
src/sync/sync-utils.ts
Outdated
const subscriptionTasks = targetTasks.find(task => task === "repository"); | ||
const repoSyncTasks = targetTasks.filter(task => task !== "repository"); | ||
const updateRepoSyncTasks = {}; | ||
const updateSubscriptionTasks: subscriptionUpdateTasks = {}; |
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.
I'd probbly move this in the if statement below and start it with the default key repositoryStatus: null
What's in this PR?
During a targetted backfill reset the relevant cursors and status's in reposync table
Why
Stops use syncing extra stuff we don't want to or setting states out of sync !