Skip to content
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

[non-Gallery] Add a non-locked monitoring cursor for inspection during backfill job #8577

Merged
merged 1 commit into from
May 17, 2021

Conversation

drewgillies
Copy link
Contributor

This makes monitoring a large-scale collect/update job a lot easier to track--it writes a monitoring cursor (date only) file every time it updates the locked cursor file. Not being locked, the monitoring cursor can be inspected randomly to determine progress.

@drewgillies drewgillies requested a review from a team as a code owner May 10, 2021 00:55
@drewgillies drewgillies changed the title [non-Gallery] Add a non-locked monitoring cursor for inspection during job [non-Gallery] Add a non-locked monitoring cursor for inspection during backfill job May 10, 2021
// Write a monitoring cursor (not locked) so for a large job we can inspect progress
if (!string.IsNullOrEmpty(MonitoringCursorFileName))
{
File.WriteAllText(MonitoringCursorFileName, cursorTime.Value.ToShortDateString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd probably be more future proof just to write the full cursor value. Who knows when we'll need that more precise value.

@@ -399,6 +407,12 @@ private async Task CommitBatch(EntitiesContext context, FileCursor cursor, Logge
if (cursorTime.HasValue)
{
await cursor.Write(cursorTime.Value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we modify FileCursor to have this same behavior of not keeping the file handle open forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about this and thought this was a less intrusive fix. But I could take a look.

@joelverhagen
Copy link
Member

Console.WriteLine could be used to the similar affect, right?

@drewgillies
Copy link
Contributor Author

Console.WriteLine could be used to the similar affect, right?

@joelverhagen the problem there is when you no longer have access to the user session that initiated the process. You can only view the users tab in the task manager to see if it's still chewing CPU, and there's no available console.out.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. If the file cursor change is too much work, don't worry about it.

@@ -275,6 +277,12 @@ public async Task Update(SqlConnection connection, string fileName)
{
await CommitBatch(context, cursor, logger, metadata.Created);
counter = 0;

// Write a monitoring cursor (not locked) so for a large job we can inspect progress
if (!string.IsNullOrEmpty(MonitoringCursorFileName))
Copy link
Contributor

@zhhyu zhhyu May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little curious about the reason to log the cursor time again here, because it seems that "CommitBatch" at line 278 has already logged it, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, @zhhyu --this is a double-up. I've moved this piece of logic to the collect method.

@drewgillies drewgillies force-pushed the dg-backfill-addmonitoringcursor branch from 8c65bb5 to 8830651 Compare May 17, 2021 01:10
@drewgillies drewgillies merged commit 77c4e24 into dev May 17, 2021
@drewgillies drewgillies deleted the dg-backfill-addmonitoringcursor branch May 17, 2021 01:31
@lyndaidaii lyndaidaii mentioned this pull request May 18, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants