Make ProjectUtils.waitForProjects() more robust by briandealwis · Pull Request #1552 · GoogleCloudPlatform/google-cloud-eclipse

briandealwis · 2017-03-08T16:40:29Z

ProjectUtils#waitForProjects() has a few issues:

I encountered a live lock problem with Associate GCP Projects with perspective #1455 where ProjectUtils#waitForProjects() would spin trying to join on the workbench's Auto-Build job.
Separately @chanseokoh pointed me at a hung Travis build .
It would spin until all build errors were resolved, which results in tests eventually timing out if those errors are never resolved (e.g., because of network resolution errors outside of our control)

This change ensures:

we only wait on runnable Jobs
we track whether the build errors have reached a fixpoint
we output debug information after 10s have elapsed (could tune this further)

akerekes · 2017-03-08T18:18:26Z

+        // track whether we've reached a fixpoint in the build errors
        allBuildErrors = getAllBuildErrors(projects);
-        if (DEBUG) {
+        buildErrorsChanging =


Would it make sense to try to check whether the build errors are the same as in the previous round instead of just the change in the count?

It would. I should just put them into a Set and do a comparison with equals().

akerekes · 2017-03-08T18:23:40Z

@@ -184,9 +187,12 @@ public void run() {
  /** Wait for any spawned jobs and builds to complete (e.g., validation jobs). */
  public static void waitForProjects(Runnable delayTactic, IProject... projects) {


This method is pretty long, I suggest to break out parts into helper methods:

collecting jobs (including filtering on line 214-219

logic when more than 60 seconds has passed

chanseokoh · 2017-03-08T18:24:01Z

        Collections.addAll(jobs, jobManager.find("org.eclipse.wst.server.ui.family"));
        Collections.addAll(jobs, jobManager.find(ValidationBuilder.FAMILY_VALIDATION_JOB));
+
+        // Remove SLEEPING jobs as they may not run for a long time.


Tests are failing reliably, and I wonder if it has something to do with this. The Javadoc of Job.schedule() says

* Schedules this job to be run after a specified delay. The job is put in the * {@link #SLEEPING} state until the specified delay has elapsed, after which * the job is added to a queue of {@link #WAITING} jobs.

So, if an important job is scheduled with a delay, there is a chance we don't catch it. Not sure though.

chanseokoh · 2017-03-08T18:27:55Z

+              }
+            }
+          }
+          if (timer.elapsed(TimeUnit.SECONDS) > 60) {


So, if we go over 60 seconds, I guess we will print the state very frequently, every time we loop? A small interval might be a good idea.

Good point.

briandealwis · 2017-03-08T19:27:26Z

There's something special about the autobuild jobs.

codecov-io · 2017-03-08T20:05:37Z

Codecov Report

Merging #1552 into master will increase coverage by 0.73%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #1552      +/-   ##
============================================
+ Coverage     70.53%   71.27%   +0.73%     
- Complexity     1307     1341      +34     
============================================
  Files           233      233              
  Lines          8995     9198     +203     
  Branches        765      793      +28     
============================================
+ Hits           6345     6556     +211     
+ Misses         2326     2322       -4     
+ Partials        324      320       -4

Impacted Files	Coverage Δ	Complexity Δ
.../tools/eclipse/test/util/project/ProjectUtils.java	`88.88% <83.33%> (+9.29%)`	`23 <13> (+5)`	✅
...gine/deploy/ui/StandardDeployPreferencesPanel.java	`87.84% <0%> (+2.03%)`	`33% <0%> (+10%)`	✅
...m/google/cloud/tools/eclipse/ui/util/FontUtil.java	`79.16% <0%> (+2.69%)`	`6% <0%> (+1%)`	✅
...tools/eclipse/projectselector/ProjectSelector.java	`92.42% <0%> (+2.76%)`	`9% <0%> (ø)`	❌
...gle/cloud/tools/eclipse/ui/util/WorkbenchUtil.java	`76.92% <0%> (+4.92%)`	`6% <0%> (+2%)`	✅
...clipse/appengine/facets/NonSystemJobSuspender.java	`89.18% <0%> (+5.4%)`	`10% <0%> (+1%)`	✅
...ols/eclipse/usagetracker/AnalyticsPingManager.java	`70.62% <0%> (+14.62%)`	`27% <0%> (+11%)`	✅
...pengine/validation/AbstractXmlSourceValidator.java	`92% <0%> (+40%)`	`8% <0%> (+4%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b38726...96d2499. Read the comment docs.

- minimize output to what's necessary - do proper build-error comparisons

briandealwis · 2017-03-09T19:00:47Z

PTAL @akerekes @chanseokoh

chanseokoh · 2017-03-09T20:30:32Z

+        buildErrorsChanging = !previousBuildErrors.equals(currentBuildErrors);
+        previousBuildErrors = currentBuildErrors;
+
+        if (DEBUG || timer.elapsed(TimeUnit.SECONDS) > 10) {


I hope this doesn't print out too many messages. Once we go over 10 seconds, we log every time we loop.

But we sleep, and we only log for the conditions that keep us in the loop which is important info.

Make ProjectUtils.waitForProject() more robust

bec782b

googlebot added the cla: yes label Mar 8, 2017

akerekes reviewed Mar 8, 2017

View reviewed changes

chanseokoh reviewed Mar 8, 2017

View reviewed changes

Back out discarding of SLEEPING jobs to enable autobuild

91fcbcb

- refactor waitForProjects() a bit

96d2499

- minimize output to what's necessary - do proper build-error comparisons

chanseokoh approved these changes Mar 9, 2017

View reviewed changes

chanseokoh reviewed Mar 9, 2017

View reviewed changes

briandealwis merged commit accae1c into master Mar 9, 2017

briandealwis deleted the waitforproject branch March 9, 2017 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ProjectUtils.waitForProjects() more robust#1552

Make ProjectUtils.waitForProjects() more robust#1552
briandealwis merged 3 commits intomasterfrom
waitforproject

briandealwis commented Mar 8, 2017

Uh oh!

akerekes Mar 8, 2017

Uh oh!

briandealwis Mar 8, 2017

Uh oh!

akerekes Mar 8, 2017

Uh oh!

chanseokoh Mar 8, 2017

Uh oh!

chanseokoh Mar 8, 2017

Uh oh!

briandealwis Mar 8, 2017

Uh oh!

briandealwis commented Mar 8, 2017

Uh oh!

codecov-io commented Mar 8, 2017 •

edited

Loading

Uh oh!

briandealwis commented Mar 9, 2017

Uh oh!

chanseokoh Mar 9, 2017

Uh oh!

briandealwis Mar 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -184,9 +187,12 @@ public void run() {
		/** Wait for any spawned jobs and builds to complete (e.g., validation jobs). */
		public static void waitForProjects(Runnable delayTactic, IProject... projects) {

Conversation

briandealwis commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briandealwis commented Mar 8, 2017

Uh oh!

codecov-io commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

briandealwis commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Mar 8, 2017 •

edited

Loading