vendor directory writes: add counts to verbose logging, limits writers, abort on error #1043

jmank88 · 2017-08-21T11:56:42Z

What does this do / why do we need it?

This change adds counts to some of the verbose logging lists:

dry-run vendor directories (Would have written..)
normal run vendor directories (Wrote.../Failed to write..., previously logged Writing... prior to work)
normal run vendor errors list (Failed to write dep tree...)

2 is concurrent and I/O limited, so this should help communicate progress to the user. 1 and 3 log immediately, but I still think the count/total is valuable for reference, grouping, and just scrolling through a really long list.

Example:

...
(46/84) Wrote github.com/pelletier/[email protected]
(47/84) Wrote github.com/go-openapi/validate@master
(48/84) Wrote github.com/golang/groupcache@master
(49/84) Wrote github.com/go-openapi/analysis@master
(50/84) Wrote github.com/sirupsen/[email protected]
(51/84) Wrote github.com/gorilla/[email protected]
(52/84) Wrote github.com/cactus/[email protected]
(53/84) Wrote github.com/urfave/[email protected]
(54/84) Wrote github.com/spf13/afero@master
...

Edit: Second commit refactors the concurrency for limiting writers and aborting on error.

What should your reviewer look out for in this PR?

concurrency simplifications
better logs messages
more places to add counts

Which issue(s) does this PR fix?

Follow up from #1037

jmank88 · 2017-08-21T12:00:47Z

internal/gps/identifier.go

@@ -148,6 +148,10 @@ func (i ProjectIdentifier) errString() string {
 	return fmt.Sprintf("%s (from %s)", i.ProjectRoot, i.Source)
 }

+func (i ProjectIdentifier) String() string {
+	return i.errString()


Any objections to this errString format becoming the canonical exported String method format? fmt.Sprintf("%s (from %s)", i.ProjectRoot, i.Source)

errString() has 36 usages, all of which are passed to formatting functions with %s, so I'd like to do a follow up PR to just absorb that method into this one and let them all implicitly call this.

mmm yes, i like this. the individual properties are accessible already - callers can easily construct their own output if they so choose. 👍

(separate PR, though)

ibrasho · 2017-08-21T13:22:02Z

One thing I regret not doing in retrospective, especially after going through #903, is that we really want to stop writing vendor as soon as an error occurs (since continuing is useless). This impacts logging somehow, but we can merge these changes and apply that after.

ibrasho · 2017-08-22T17:09:44Z

internal/gps/result.go

+
+			var err error
+			defer func() {
+				if r := recover(); r != nil {


Do we get panics here?

I haven't seen one, but I wanted to be careful not to return a false positive if one occurs.

jmank88 · 2017-08-27T14:53:48Z

One thing I regret not doing in retrospective, especially after going through #903, is that we really want to stop writing vendor as soon as an error occurs (since continuing is useless). This impacts logging somehow, but we can merge these changes and apply that after.

I went ahead and added this because I was playing around with it and, although it's more complex, it actually ended up cleaner (e.g. no more recover()), plus we can control the number of concurrent writers.

jmank88 · 2017-08-28T12:30:19Z

internal/gps/result.go

+	}
+	close(writeCh)
+	// Launch writers.
+	writers := runtime.GOMAXPROCS(-1)


Perhaps an optimal number is some factor of GOMAXPROCS, or ultimately something configurable.
Related discussion on how this might be configurable: #1028 (comment)

i'm not sure that GOMAXPROCS is the right thing, here - the goroutines themselves aren't actually doing work, but end up mostly in a sleep mode waiting for their spawned subprocesses. limiting it to GOMAXPROCS seems likely to be lower than we want.

i think i'd rather just pick an arbitrary number to begin with - let's say 16 - and see how we do.

sdboyer

looks mostly good

sdboyer · 2017-08-29T03:38:50Z

internal/gps/result.go

+	}
+	close(writeCh)
+	// Launch writers.
+	writers := runtime.GOMAXPROCS(-1)


i'm not sure that GOMAXPROCS is the right thing, here - the goroutines themselves aren't actually doing work, but end up mostly in a sleep mode waiting for their spawned subprocesses. limiting it to GOMAXPROCS seems likely to be lower than we want.

i think i'd rather just pick an arbitrary number to begin with - let's say 16 - and see how we do.

sdboyer · 2017-08-29T03:59:01Z

internal/gps/result.go

+		msg := "Wrote"
+		if resp.err != nil {
+			if len(errs) == 0 {
+				close(cancel)


need to know to break out of the loop explicitly here. i believe there's a possibility of a double-close on the cancel channel:

X and Y are all sent into the writeCh, and are all picked up by their own goroutine

X finishes first and fails, indicates as much to the respCh; it terminates, decrementing the wg

main goroutine receives the fail and closes cancel, logs result, returns to loop and waits for new value

meanwhile, Y finishes and also fails, sends to respCh, and terminates, closing respCh from the waiter goroutine

however, main goroutine is still waiting on the respCh and is already working before it receives second fail, panics on double-close

note that because the range loop will not terminate on a buffered channel until the chan is both closed AND empty, i believe this is guaranteed to happen if two workers simultaneously encounter an error.

The close(cancel) is protected by len(errs) == 0 so that the channel is only closed on the first error. We could break and abort on the first error (and only report that single error), but since we want to gracefully block/wait/shutdown anyways, it seemed cleaner to inspect all responses and collect all errors (e.g. might be relevant to investigating the original error; we don't want to block/wait in silence then not log anything; etc...).

gah, you're right, i just breezed right by that check. ok 👍

verbose logging: add counts to vendor directory lists

76e1854

jmank88 requested a review from sdboyer as a code owner August 21, 2017 11:56

googlebot added the cla: yes label Aug 21, 2017

jmank88 commented Aug 21, 2017

View reviewed changes

jmank88 requested review from ibrasho and carolynvs August 21, 2017 12:01

ibrasho reviewed Aug 22, 2017

View reviewed changes

limit # routines writing dep tree; abort on error

ef47c48

jmank88 changed the title ~~verbose logging: add counts to vendor directory lists~~ vendor directory writes: add counts to verbose logging, limits writers, abort on error Aug 27, 2017

jmank88 commented Aug 28, 2017

View reviewed changes

sdboyer suggested changes Aug 29, 2017

View reviewed changes

gps: change WriteDepTree concurrent writers from GOMAXPROCS to 16

1b50999

jmank88 closed this Aug 29, 2017

jmank88 reopened this Aug 29, 2017

carolynvs removed their request for review August 29, 2017 13:23

sdboyer approved these changes Aug 30, 2017

View reviewed changes

sdboyer merged commit 712e606 into golang:master Aug 30, 2017

jmank88 deleted the verbose_counts branch August 30, 2017 16:16

jmank88 mentioned this pull request Aug 30, 2017

gps: ProjectIdentifier: drop errString()method in favor of implicit String() calls #1096

Merged

ibrasho added the area: gps label Sep 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vendor directory writes: add counts to verbose logging, limits writers, abort on error #1043

vendor directory writes: add counts to verbose logging, limits writers, abort on error #1043

jmank88 commented Aug 21, 2017 •

edited

Loading

jmank88 Aug 21, 2017

sdboyer Aug 27, 2017

sdboyer Aug 29, 2017

ibrasho commented Aug 21, 2017 •

edited

Loading

ibrasho Aug 22, 2017

jmank88 Aug 22, 2017

jmank88 commented Aug 27, 2017 •

edited

Loading

jmank88 Aug 28, 2017

sdboyer Aug 29, 2017

sdboyer left a comment

sdboyer Aug 29, 2017

sdboyer Aug 29, 2017

jmank88 Aug 29, 2017 •

edited

Loading

sdboyer Aug 30, 2017

vendor directory writes: add counts to verbose logging, limits writers, abort on error #1043

vendor directory writes: add counts to verbose logging, limits writers, abort on error #1043

Conversation

jmank88 commented Aug 21, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 commented Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 commented Aug 21, 2017 •

edited

Loading

ibrasho commented Aug 21, 2017 •

edited

Loading

jmank88 commented Aug 27, 2017 •

edited

Loading

jmank88 Aug 29, 2017 •

edited

Loading