Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(84)

Issue 2583033002: LogDog: Re-use Pub/Sub gRPC clients. (Closed)

Created:
4 years ago by dnj
Modified:
4 years ago
Reviewers:
Vadim Sh.
CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org
Target Ref:
refs/heads/master
Project:
luci-go
Visibility:
Public.

Description

LogDog: Re-use Pub/Sub gRPC clients. Since a single Tumble mutation may perform multiple Pub/Sub publishes, re-use a single Pub/Sub gRPC client. Clients are bound to the projects that they are created from, and free'd at the end of the handler's operation via a service-wide close. Leave the archival publisher Close in (even though it's no-op) since it's a useful check on archival publisher lifecycle. BUG=chromium:675401 TEST=None R=vadimsh@chromium.org Review-Url: https://codereview.chromium.org/2583033002 Committed: https://github.com/luci/luci-go/commit/fad4dadaa7d4d99fc00958a4d5281af7f9d85a7c

Patch Set 1 #

Total comments: 2

Patch Set 2 : close under lock #

Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -21 lines) Patch
M logdog/appengine/coordinator/archivalPublisher.go View 3 chunks +5 lines, -4 lines 0 comments Download
M logdog/appengine/coordinator/service.go View 1 6 chunks +63 lines, -17 lines 0 comments Download

Messages

Total messages: 9 (4 generated)
dnj
PTAL
4 years ago (2016-12-18 22:53:02 UTC) #1
Vadim Sh.
lgtm with optional nit https://codereview.chromium.org/2583033002/diff/1/logdog/appengine/coordinator/service.go File logdog/appengine/coordinator/service.go (right): https://codereview.chromium.org/2583033002/diff/1/logdog/appengine/coordinator/service.go#newcode130 logdog/appengine/coordinator/service.go:130: for proj, client := range ...
4 years ago (2016-12-19 19:59:45 UTC) #2
dnj
https://codereview.chromium.org/2583033002/diff/1/logdog/appengine/coordinator/service.go File logdog/appengine/coordinator/service.go (right): https://codereview.chromium.org/2583033002/diff/1/logdog/appengine/coordinator/service.go#newcode130 logdog/appengine/coordinator/service.go:130: for proj, client := range s.pubSubClients { On 2016/12/19 ...
4 years ago (2016-12-19 21:43:55 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2583033002/20001
4 years ago (2016-12-19 21:44:19 UTC) #6
commit-bot: I haz the power
4 years ago (2016-12-19 21:50:35 UTC) #9
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/fad4dadaa7d4d99fc00958a4d5281af7f9d85a7c

Powered by Google App Engine
This is Rietveld 408576698