Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

Concurrent in-flight requests to Stackdriver Logging #193

Merged

Conversation

fluffle
Copy link
Collaborator

@fluffle fluffle commented Feb 8, 2018

Thanks to some great work by @jba we can now have multiple in-flight requests to Stackdriver. This should make Nozzle throughput much less susceptible to latency-induced drops.

This PR pulls in the vendor changes and adds a new config option so that the concurrency limit is tuneable (default: 16).


This change is Reviewable

@fluffle fluffle requested review from johnsonj and knyar February 8, 2018 13:30
@knyar
Copy link
Collaborator

knyar commented Feb 8, 2018

Looks reasonable, but let's wait for @johnsonj to lgtm.

@knyar
Copy link
Collaborator

knyar commented Feb 8, 2018

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


jobs/stackdriver-nozzle/spec, line 56 at r1 (raw file):

    default: 30

  nozzle.logging_requests_in_flight

Colon missing here.


Comments from Reviewable

@johnsonj
Copy link
Contributor

johnsonj commented Feb 9, 2018

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@johnsonj
Copy link
Contributor

johnsonj commented Feb 9, 2018

:lgtm: pending Anton's comment


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@fluffle fluffle force-pushed the pr-vendor-parallel-logging branch from 963dd90 to 0dd1e66 Compare February 9, 2018 10:23
@fluffle
Copy link
Collaborator Author

fluffle commented Feb 9, 2018

This "stale LGTM" feature really breaks the "LGTM once you do X, Y, Z" work pattern that is basically necessary for reasonable cross-continent development velocity. I wonder if there's any way of telling Reviewable to STFU once an LGTM is given.

@fluffle
Copy link
Collaborator Author

fluffle commented Feb 9, 2018

Review status: 10 of 11 files reviewed at latest revision, 1 unresolved discussion.


jobs/stackdriver-nozzle/spec, line 56 at r1 (raw file):

Previously, knyar (Anton Tolchanov) wrote…

Colon missing here.

Done.


Comments from Reviewable

@knyar
Copy link
Collaborator

knyar commented Feb 9, 2018

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@knyar knyar merged commit 1fd4066 into cloudfoundry-community:develop Feb 9, 2018
@johnsonj johnsonj mentioned this pull request Feb 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants