-
Notifications
You must be signed in to change notification settings - Fork 59
update the watchdog file #440
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
Conversation
Pull Request Test Coverage Report for Build 1562
💛 - Coveralls |
|
@allieychen Can you put the commentary about why this change is being made in the commit itself? It's much better to have information like this in the commit object rather than the PR since the PR is not a permanent part of the record. |
nmousavi
left a comment
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.
Can we separate file update from runners logic, perhaps by running an updater in different thread?
I don't like the fact that update is done in multiple places, it's prone to bug and ugly. Any future change that adds a costly processing step here would require another manual update (similar to what you added in this PR), and it can be easily missed out.
|
Thanks for the review! I have moved updating the watchdog file in another thread. PTAL :) |
2309da2 to
779be0b
Compare
nmousavi
left a comment
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.
Please test it on GCP, and mentioned it in the description.
Once done LGTM.
Fix one bug for annotation. The operations could get killed because of the watchdog file is stale even though no cancellation is made. The watchdog file was updated only when waiting for one operation to be done. This fix handles the following two failing cases:
Tested: unit tests & integration tests & manually ran one annotation test.