Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981683002:
Milo: Move buildbucket pubsub sub from buildbucket project to milo project (Closed)
|
Created:
3 years, 5 months ago by Ryan Tseng Modified:
3 years, 5 months ago 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. |
DescriptionMilo: Move buildbucket pubsub sub from buildbucket project to milo project
This moves (for instance) the subscription from:
projects/cr-buildbucket/subscriptions/luci-milo
to:
projects/luci-milo/subscriptions/buildbucket
This removes a series of complex steps when setting up the pubsub pipeline.
Nothing else changes.
BUG=624960
Review-Url: https://codereview.chromium.org/2981683002
Committed: https://github.com/luci/luci-go/commit/4bfe11ebfbb203db86104241a7a748df16f507ef
Patch Set 1 #Patch Set 2 : subID -> appID #
Total comments: 6
Patch Set 3 : review #
Total comments: 11
Patch Set 4 : Fixes #
Total comments: 10
Patch Set 5 : Use factories #Patch Set 6 : luciErrors -> errors #Patch Set 7 : luciErrors -> errors #
Total comments: 5
Patch Set 8 : Make factory just a type signature #Patch Set 9 : comments #Messages
Total messages: 56 (32 generated)
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hinoka@google.com changed reviewers: + iannucci@chromium.org, nodir@chromium.org
ptal. As per vadim's suggestion, this removes that annoying 4 step process for webmaster verification.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:100: return nil, fmt.Errorf("no pubsub clients installed for" + projectID) space missing ...for %s" https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:121: clients[projectID] = &prodPubSubClient{c, client} it used to be gouroutine safe, but now it is not. Typically functions that deal with context are goroutine safe. You need to RWMutex on the map https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:231: logging.WithError(err).Errorf(c, "could not create subscription %#v", sub) why do we both log error and return it? the caller is likely to log it too, so the error would be double-logged. consider adding the context ("could not create subscription...") to the error itself (perhaps using errors.Annotate) and return it without logging.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:100: return nil, fmt.Errorf("no pubsub clients installed for" + projectID) On 2017/07/12 21:31:12, nodir wrote: > space missing > > ...for %s" Done. https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:121: clients[projectID] = &prodPubSubClient{c, client} On 2017/07/12 21:31:12, nodir wrote: > it used to be gouroutine safe, but now it is not. Typically functions that deal > with context are goroutine safe. > > You need to RWMutex on the map Done. https://codereview.chromium.org/2981683002/diff/20001/milo/common/pubsub.go#n... milo/common/pubsub.go:231: logging.WithError(err).Errorf(c, "could not create subscription %#v", sub) On 2017/07/12 21:31:12, nodir wrote: > why do we both log error and return it? the caller is likely to log it too, so > the error would be double-logged. > > consider adding the context ("could not create subscription...") to the error > itself (perhaps using errors.Annotate) and return it without logging. Done.
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:65: lock: sync.RWMutex{}, this line is noop, unnecessary https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} this may cause a memory leak. Consider: - there is a chain of contexts: c1 <- c2 <- c3 <- c4 c1 is some global context while c4 is specific to a request - c1 contains the client bundle - c == c4 - the pubsubClient is created with c4 and is put into the bundle. Now the global c1 has a references to request-specific c4 - future requests use the pubsubClient bound to the c4. This may cause all sorts of problems I am not sure prodPubSubClient should store context. Typically context is passed in functions as the first parameter, as you know. For example, if a client wants to set a timeout or pass metadata, they create an configured context and pass in a function call, as opposed to reconfigure entire client object (which can be global, like in this case). Consider removing context from the prodPubSubClient and passing context in each of its methods. https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:197: luciErrors.Annotate(err, "%s does not exist", topicID).Err() this is noop because the return value of Err() is not used it should be returned
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:65: lock: sync.RWMutex{}, On 2017/07/12 23:06:58, nodir wrote: > this line is noop, unnecessary Oh you're right, removed. https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/12 23:06:58, nodir wrote: > this may cause a memory leak. Consider: > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > c1 is some global context while c4 is specific to a request > - c1 contains the client bundle > - c == c4 > - the pubsubClient is created with c4 and is put into the bundle. Now the global > c1 has a references to request-specific c4 > - future requests use the pubsubClient bound to the c4. This may cause all sorts > of problems > > I am not sure prodPubSubClient should store context. Typically context is passed > in functions as the first parameter, as you know. For example, if a client wants > to set a timeout or pass metadata, they create an configured context and pass in > a function call, as opposed to reconfigure entire client object (which can be > global, like in this case). > > Consider removing context from the prodPubSubClient and passing context in each > of its methods. I see, you're right. I used this pattern only because I've seen it used in other places but the concern is valid. https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:197: luciErrors.Annotate(err, "%s does not exist", topicID).Err() On 2017/07/12 23:06:58, nodir wrote: > this is noop because the return value of Err() is not used > > it should be returned my bad, thanks.
fwiw, this lgtm, though I have less understanding of the pubsub interactions than most. https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:16: luciErrors "github.com/luci/luci-go/common/errors" just use this instead of errors. there's no reason to import both. https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:21: var pubSubClientKey = "stores a pubsubClient" *pubsubClients ?
https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:125: client, err := pubsub.NewClient(c, projectID) Just noticed c here. Memory leakish problem happens here too if EnsurePubSubSubscribed is the only caller of this function, it might be better to put the filled map without a mutex into the context right there without withClient's generality (which seems to be hard to implement correctly) However, a global pubsub client can be problematic. After reading pubsub code a bit, I learned that if someone calls http://godoc.org/cloud.google.com/go/pubsub#Client.Close on a global client, it will affect all future requests that use the client Stepping back, I assume clients are in the context because you want to use stubs in tests. Same result could be achieved by a simpler approach: store a func(c context.Context, projectID string) pubSubClient In context. In getPubSubClient, check if function is there. If it is, use it, otherwise create a prod instance. Then you don't need pubsubClients, the map, the mutex, newClientsBundle or withClient. You wouldn't really need getTopic, getSubscription and createSubscription either. You could call getPubSubClient once and then use its methods directly. I think overall it would lead to less code Up to you https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:136: clients.clients[projectID] = &prodPubSubClient{client} What if it is already there?
https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:57: type pubsubClients struct { now that I'm looking at this again... this is a bit weird. Can we just make the clients on demand?
https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:125: client, err := pubsub.NewClient(c, projectID) On 2017/07/13 02:07:11, nodir wrote: > Just noticed c here. Memory leakish problem happens here too > > if EnsurePubSubSubscribed is the only caller of this function, it might be > better to put the filled map without a mutex into the context right there > without withClient's generality (which seems to be hard to implement correctly) > > However, a global pubsub client can be problematic. After reading pubsub code a > bit, I learned that if someone calls > http://godoc.org/cloud.google.com/go/pubsub#Client.Close on a global client, it > will affect all future requests that use the client > > Stepping back, I assume clients are in the context because you want to use stubs > in tests. Same result could be achieved by a simpler approach: store a func(c > context.Context, projectID string) pubSubClient > In context. > > In getPubSubClient, check if function is there. If it is, use it, otherwise > create a prod instance. Then you don't need pubsubClients, the map, the mutex, > newClientsBundle or withClient. You wouldn't really need getTopic, > getSubscription and createSubscription either. You could call getPubSubClient > once and then use its methods directly. I think overall it would lead to less > code > > Up to you The client is in the context because of the guideline: https://godoc.org/cloud.google.com/go/pubsub#Client "Clients should be reused rather than being created as needed" I assume that is because creating a client also creates 2 RPC connection dials: https://github.com/GoogleCloudPlatform/google-cloud-go/blob/master/pubsub/api... https://github.com/GoogleCloudPlatform/google-cloud-go/blob/master/pubsub/api... We do reuse the client for multiple calls, so creating multiple connection for each call wouldn't be super elegant. I'm not super worried about the global .Close() if we don't expose the underlying client. The embedded context is unfortunate though, since there's no other way to reuse an existing client than to hold a reference to the context at that time https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:136: clients.clients[projectID] = &prodPubSubClient{client} On 2017/07/13 02:07:11, nodir wrote: > What if it is already there? it's overwritten
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/13 01:08:26, Ryan Tseng wrote: > On 2017/07/12 23:06:58, nodir wrote: > > this may cause a memory leak. Consider: > > > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > > c1 is some global context while c4 is specific to a request > > - c1 contains the client bundle > > - c == c4 > > - the pubsubClient is created with c4 and is put into the bundle. Now the > global > > c1 has a references to request-specific c4 > > - future requests use the pubsubClient bound to the c4. This may cause all > sorts > > of problems > > > > I am not sure prodPubSubClient should store context. Typically context is > passed > > in functions as the first parameter, as you know. For example, if a client > wants > > to set a timeout or pass metadata, they create an configured context and pass > in > > a function call, as opposed to reconfigure entire client object (which can be > > global, like in this case). > > > > Consider removing context from the prodPubSubClient and passing context in > each > > of its methods. > > I see, you're right. I used this pattern only because I've seen it used in > other places but the concern is valid. Actually on second thought, withClient() is called per request, not on instance startup. The stored clients will expire between requests, so there is no global context where the clients are stored. Am I missing anything?
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/13 02:42:57, Ryan Tseng wrote: > On 2017/07/13 01:08:26, Ryan Tseng wrote: > > On 2017/07/12 23:06:58, nodir wrote: > > > this may cause a memory leak. Consider: > > > > > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > > > c1 is some global context while c4 is specific to a request > > > - c1 contains the client bundle > > > - c == c4 > > > - the pubsubClient is created with c4 and is put into the bundle. Now the > > global > > > c1 has a references to request-specific c4 > > > - future requests use the pubsubClient bound to the c4. This may cause all > > sorts > > > of problems > > > > > > I am not sure prodPubSubClient should store context. Typically context is > > passed > > > in functions as the first parameter, as you know. For example, if a client > > wants > > > to set a timeout or pass metadata, they create an configured context and > pass > > in > > > a function call, as opposed to reconfigure entire client object (which can > be > > > global, like in this case). > > > > > > Consider removing context from the prodPubSubClient and passing context in > > each > > > of its methods. > > > > I see, you're right. I used this pattern only because I've seen it used in > > other places but the concern is valid. > > Actually on second thought, withClient() is called per request, not on instance > startup. The stored clients will expire between requests, so there is no global > context where the clients are stored. Am I missing anything? indeed, pubsub client is not global. my concern with the current patchset is that withClient is written in a generic way, but cannot be used in a generic way, i.e. one might call withClient during app initialization, and then call it again with a context-with-request and cause a memory leak. IMO code must be either generic without assumptions of non-generic usage, or it must be non-generic. Both are possible in this particular case. Personally, I don't see a reason for this generality here, e.g. I don't see a reason to put the clients into the context. You can achieve the same result by doing: - in ensurePubSubClient: add miloClient and buildbucketClient parameters - in EnsurePubSubClient: pass prodPubSubClient instances as the parameters - in tests: pass testPubSubClients instead so no need for context keys, bundles/map/mutex or functions newClientsBundle, getTopic, getSubscription, createSubscription. This would be less code and it'd still do the job. If you still insist on preserving the generality, it is still achievable in a non-problematic way. I assume you switched context value from a single client to a map of clients because you have only one context key. You could declare `type projectIDCtxKey string` and then use `projectIDCtxKey(projectID)` as a context key when putting or retrieving a client to/from a context. This way the code is still goroutine-safe and there is no need for map or mutex.
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/13 05:58:46, nodir wrote: > On 2017/07/13 02:42:57, Ryan Tseng wrote: > > On 2017/07/13 01:08:26, Ryan Tseng wrote: > > > On 2017/07/12 23:06:58, nodir wrote: > > > > this may cause a memory leak. Consider: > > > > > > > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > > > > c1 is some global context while c4 is specific to a request > > > > - c1 contains the client bundle > > > > - c == c4 > > > > - the pubsubClient is created with c4 and is put into the bundle. Now the > > > global > > > > c1 has a references to request-specific c4 > > > > - future requests use the pubsubClient bound to the c4. This may cause all > > > sorts > > > > of problems > > > > > > > > I am not sure prodPubSubClient should store context. Typically context is > > > passed > > > > in functions as the first parameter, as you know. For example, if a client > > > wants > > > > to set a timeout or pass metadata, they create an configured context and > > pass > > > in > > > > a function call, as opposed to reconfigure entire client object (which can > > be > > > > global, like in this case). > > > > > > > > Consider removing context from the prodPubSubClient and passing context in > > > each > > > > of its methods. > > > > > > I see, you're right. I used this pattern only because I've seen it used in > > > other places but the concern is valid. > > > > Actually on second thought, withClient() is called per request, not on > instance > > startup. The stored clients will expire between requests, so there is no > global > > context where the clients are stored. Am I missing anything? > > indeed, pubsub client is not global. > > my concern with the current patchset is that withClient is written in a generic > way, but cannot be used in a generic way, i.e. one might call withClient during > app initialization, and then call it again with a context-with-request and cause > a memory leak. IMO code must be either generic without assumptions of > non-generic usage, or it must be non-generic. Both are possible in this > particular case. > > Personally, I don't see a reason for this generality here, e.g. I don't see a > reason to put the clients into the context. You can achieve the same result by > doing: > - in ensurePubSubClient: add miloClient and buildbucketClient parameters > - in EnsurePubSubClient: pass prodPubSubClient instances as the parameters > - in tests: pass testPubSubClients instead > so no need for context keys, bundles/map/mutex or functions newClientsBundle, > getTopic, getSubscription, createSubscription. This would be less code and it'd > still do the job. > > If you still insist on preserving the generality, it is still achievable in a > non-problematic way. I assume you switched context value from a single client to > a map of clients because you have only one context key. You could declare `type > projectIDCtxKey string` and then use `projectIDCtxKey(projectID)` as a context > key when putting or retrieving a client to/from a context. This way the code is > still goroutine-safe and there is no need for map or mutex. I think it is very confusing to have multiple ways to reference services in the same code (both by putting them in the context and by passing them as parameters. We also use a third way in Milo which is to turn functions into methods where the class contains an overridable service provider that defaults to a production instance if not overridden. I think following multiple design patterns at the same time makes things too confusing. I think services in context is in general a confusing way to do things because it involves a ton of code spread across a lot of files and interfaces. But because we've already committed to using this pattern everywhere, I thought it was better to follow this pattern rather than invent yet another pattern, even if it was more code. Basically my primary object is to reduce complexity by limiting the number of design patterns used. The design pattern you suggested was one that I spent a long time refactoring out of in another place in the code, which is why I'm a little hesitant to use it.
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/13 18:02:30, Ryan Tseng wrote: > On 2017/07/13 05:58:46, nodir wrote: > > On 2017/07/13 02:42:57, Ryan Tseng wrote: > > > On 2017/07/13 01:08:26, Ryan Tseng wrote: > > > > On 2017/07/12 23:06:58, nodir wrote: > > > > > this may cause a memory leak. Consider: > > > > > > > > > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > > > > > c1 is some global context while c4 is specific to a request > > > > > - c1 contains the client bundle > > > > > - c == c4 > > > > > - the pubsubClient is created with c4 and is put into the bundle. Now > the > > > > global > > > > > c1 has a references to request-specific c4 > > > > > - future requests use the pubsubClient bound to the c4. This may cause > all > > > > sorts > > > > > of problems > > > > > > > > > > I am not sure prodPubSubClient should store context. Typically context > is > > > > passed > > > > > in functions as the first parameter, as you know. For example, if a > client > > > > wants > > > > > to set a timeout or pass metadata, they create an configured context and > > > pass > > > > in > > > > > a function call, as opposed to reconfigure entire client object (which > can > > > be > > > > > global, like in this case). > > > > > > > > > > Consider removing context from the prodPubSubClient and passing context > in > > > > each > > > > > of its methods. > > > > > > > > I see, you're right. I used this pattern only because I've seen it used > in > > > > other places but the concern is valid. > > > > > > Actually on second thought, withClient() is called per request, not on > > instance > > > startup. The stored clients will expire between requests, so there is no > > global > > > context where the clients are stored. Am I missing anything? > > > > indeed, pubsub client is not global. > > > > my concern with the current patchset is that withClient is written in a > generic > > way, but cannot be used in a generic way, i.e. one might call withClient > during > > app initialization, and then call it again with a context-with-request and > cause > > a memory leak. IMO code must be either generic without assumptions of > > non-generic usage, or it must be non-generic. Both are possible in this > > particular case. > > > > Personally, I don't see a reason for this generality here, e.g. I don't see a > > reason to put the clients into the context. You can achieve the same result by > > doing: > > - in ensurePubSubClient: add miloClient and buildbucketClient parameters > > - in EnsurePubSubClient: pass prodPubSubClient instances as the parameters > > - in tests: pass testPubSubClients instead > > so no need for context keys, bundles/map/mutex or functions newClientsBundle, > > getTopic, getSubscription, createSubscription. This would be less code and > it'd > > still do the job. > > > > If you still insist on preserving the generality, it is still achievable in a > > non-problematic way. I assume you switched context value from a single client > to > > a map of clients because you have only one context key. You could declare > `type > > projectIDCtxKey string` and then use `projectIDCtxKey(projectID)` as a context > > key when putting or retrieving a client to/from a context. This way the code > is > > still goroutine-safe and there is no need for map or mutex. > > I think it is very confusing to have multiple ways to reference services in the > same code (both by putting them in the context and by passing them as > parameters. We also use a third way in Milo which is to turn functions into > methods where the class contains an overridable service provider that defaults > to a production instance if not overridden. I think following multiple design > patterns at the same time makes things too confusing. > > I think services in context is in general a confusing way to do things because > it involves a ton of code spread across a lot of files and interfaces. But > because we've already committed to using this pattern everywhere, I thought it > was better to follow this pattern rather than invent yet another pattern, even > if it was more code. > > Basically my primary object is to reduce complexity by limiting the number of > design patterns used. > > The design pattern you suggested was one that I spent a long time refactoring > out of in another place in the code, which is why I'm a little hesitant to use > it. I understand that you prefer services be in context, in general. Then I have two suggestions: 1) Define a context key type (not a constant) tied to project id type projectIDCtxKey string // putting to context return ctx.WithValue(projectIDCtxKey(projectId), myNewClient) // This is goroutine safe // retrieving client = ctx.Value(projectIDCtxKey(projectId)).(pubsubClient) 2) I still don't understand why do you need getTopic, getSubscription and createSubscription wrappers. A caller can do topic = getPubSubClient(c, project).getTopic("buildbucket") Removing the wrappers will reduce amount of code by 22 lines. Not in this CL though.
https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/40001/milo/common/pubsub.go#n... milo/common/pubsub.go:137: clients.clients[projectID] = &prodPubSubClient{c, client} On 2017/07/13 19:38:39, nodir wrote: > On 2017/07/13 18:02:30, Ryan Tseng wrote: > > On 2017/07/13 05:58:46, nodir wrote: > > > On 2017/07/13 02:42:57, Ryan Tseng wrote: > > > > On 2017/07/13 01:08:26, Ryan Tseng wrote: > > > > > On 2017/07/12 23:06:58, nodir wrote: > > > > > > this may cause a memory leak. Consider: > > > > > > > > > > > > - there is a chain of contexts: c1 <- c2 <- c3 <- c4 > > > > > > c1 is some global context while c4 is specific to a request > > > > > > - c1 contains the client bundle > > > > > > - c == c4 > > > > > > - the pubsubClient is created with c4 and is put into the bundle. Now > > the > > > > > global > > > > > > c1 has a references to request-specific c4 > > > > > > - future requests use the pubsubClient bound to the c4. This may cause > > all > > > > > sorts > > > > > > of problems > > > > > > > > > > > > I am not sure prodPubSubClient should store context. Typically context > > is > > > > > passed > > > > > > in functions as the first parameter, as you know. For example, if a > > client > > > > > wants > > > > > > to set a timeout or pass metadata, they create an configured context > and > > > > pass > > > > > in > > > > > > a function call, as opposed to reconfigure entire client object (which > > can > > > > be > > > > > > global, like in this case). > > > > > > > > > > > > Consider removing context from the prodPubSubClient and passing > context > > in > > > > > each > > > > > > of its methods. > > > > > > > > > > I see, you're right. I used this pattern only because I've seen it used > > in > > > > > other places but the concern is valid. > > > > > > > > Actually on second thought, withClient() is called per request, not on > > > instance > > > > startup. The stored clients will expire between requests, so there is no > > > global > > > > context where the clients are stored. Am I missing anything? > > > > > > indeed, pubsub client is not global. > > > > > > my concern with the current patchset is that withClient is written in a > > generic > > > way, but cannot be used in a generic way, i.e. one might call withClient > > during > > > app initialization, and then call it again with a context-with-request and > > cause > > > a memory leak. IMO code must be either generic without assumptions of > > > non-generic usage, or it must be non-generic. Both are possible in this > > > particular case. > > > > > > Personally, I don't see a reason for this generality here, e.g. I don't see > a > > > reason to put the clients into the context. You can achieve the same result > by > > > doing: > > > - in ensurePubSubClient: add miloClient and buildbucketClient parameters > > > - in EnsurePubSubClient: pass prodPubSubClient instances as the parameters > > > - in tests: pass testPubSubClients instead > > > so no need for context keys, bundles/map/mutex or functions > newClientsBundle, > > > getTopic, getSubscription, createSubscription. This would be less code and > > it'd > > > still do the job. > > > > > > If you still insist on preserving the generality, it is still achievable in > a > > > non-problematic way. I assume you switched context value from a single > client > > to > > > a map of clients because you have only one context key. You could declare > > `type > > > projectIDCtxKey string` and then use `projectIDCtxKey(projectID)` as a > context > > > key when putting or retrieving a client to/from a context. This way the code > > is > > > still goroutine-safe and there is no need for map or mutex. > > > > I think it is very confusing to have multiple ways to reference services in > the > > same code (both by putting them in the context and by passing them as > > parameters. We also use a third way in Milo which is to turn functions into > > methods where the class contains an overridable service provider that defaults > > to a production instance if not overridden. I think following multiple design > > patterns at the same time makes things too confusing. > > > > I think services in context is in general a confusing way to do things because > > it involves a ton of code spread across a lot of files and interfaces. But > > because we've already committed to using this pattern everywhere, I thought it > > was better to follow this pattern rather than invent yet another pattern, even > > if it was more code. > > > > Basically my primary object is to reduce complexity by limiting the number of > > design patterns used. > > > > The design pattern you suggested was one that I spent a long time refactoring > > out of in another place in the code, which is why I'm a little hesitant to use > > it. > > I understand that you prefer services be in context, in general. Then I have two > suggestions: > > 1) Define a context key type (not a constant) tied to project id > > type projectIDCtxKey string > > // putting to context > return ctx.WithValue(projectIDCtxKey(projectId), myNewClient) > // This is goroutine safe > > // retrieving > client = ctx.Value(projectIDCtxKey(projectId)).(pubsubClient) > > 2) I still don't understand why do you need getTopic, getSubscription and > createSubscription wrappers. A caller can do > > topic = getPubSubClient(c, project).getTopic("buildbucket") > > Removing the wrappers will reduce amount of code by 22 lines. > > Not in this CL though. another issue with the current patchset is: c1 := context.Background() c2 := withClient(c1, "a") withClient(c2, "b") // this mutates c2 getPubSubClient(c2, "b") is not nil this happens because a mutable object (map) is put into the a context value. can be solved by the suggestion (1) above
Discussed briefly with iannucci, decided to install a pubsub client factory into the context instead of the client because: * This still follows the "testing stuff in context" pattern * It's up to the caller to manage clients, and the context remains non-mutable and also does not nest contexts This was the original approach also in https://codereview.chromium.org/2955223002/ which was changed to a client-in-context approach because it looked better, but then it comes with complexity. The factory may use either the current client or the thin client: https://godoc.org/google.golang.org/api/pubsub/v1 Either way it'll do the same thing, but the thin client requires more time to research.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal latest patchset. It uses client factories in context instead of clients in context. I think this is a simpler approach and addresses concurrency issues. The downside is the inability to share previously dialed clients in the context with other code, but that was never an issue to begin with anyways. https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:16: luciErrors "github.com/luci/luci-go/common/errors" On 2017/07/13 01:54:02, iannucci wrote: > just use this instead of errors. there's no reason to import both. Done. https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:21: var pubSubClientKey = "stores a pubsubClient" On 2017/07/13 01:54:02, iannucci wrote: > *pubsubClients ? Done. https://codereview.chromium.org/2981683002/diff/60001/milo/common/pubsub.go#n... milo/common/pubsub.go:57: type pubsubClients struct { On 2017/07/13 02:16:27, iannucci wrote: > now that I'm looking at this again... this is a bit weird. Can we just make the > clients on demand? Made this back into a client factory
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go#... milo/common/pubsub.go:57: type pubsubClientFactory interface { wouldn't type pubsubClientFactory func(ctx context.Context, projectID string) (pubsubClient, error) be simpler? You wouldn't need prodPubSubClientFactory as a struct https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go#... milo/common/pubsub.go:105: // given project ID this comment is stale
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go#... milo/common/pubsub.go:57: type pubsubClientFactory interface { On 2017/07/14 01:09:04, nodir wrote: > wouldn't > > type pubsubClientFactory func(ctx context.Context, projectID string) > (pubsubClient, error) > > > be simpler? You wouldn't need prodPubSubClientFactory as a struct Thanks, I forgot this was legal edit: this is possible but there is a trap (I learned today), Bound methods will not match this type interface, but a closed function will. I made a comment about that in the test code. https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go#... milo/common/pubsub.go:105: // given project ID On 2017/07/14 01:09:04, nodir wrote: > this comment is stale Done.
The CQ bit was unchecked by hinoka@google.com
The CQ bit was checked by hinoka@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org, nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2981683002/#ps160001 (title: "comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1500054117931080,
"parent_rev": "915ce57f5cd2262f6324a5af4bf7643f566e1dea", "commit_rev":
"4bfe11ebfbb203db86104241a7a748df16f507ef"}
Message was sent while issue was closed.
Description was changed from ========== Milo: Move buildbucket pubsub sub from buildbucket project to milo project This moves (for instance) the subscription from: projects/cr-buildbucket/subscriptions/luci-milo to: projects/luci-milo/subscriptions/buildbucket This removes a series of complex steps when setting up the pubsub pipeline. Nothing else changes. BUG=624960 ========== to ========== Milo: Move buildbucket pubsub sub from buildbucket project to milo project This moves (for instance) the subscription from: projects/cr-buildbucket/subscriptions/luci-milo to: projects/luci-milo/subscriptions/buildbucket This removes a series of complex steps when setting up the pubsub pipeline. Nothing else changes. BUG=624960 Review-Url: https://codereview.chromium.org/2981683002 Committed: https://github.com/luci/luci-go/commit/4bfe11ebfbb203db86104241a7a748df16f507ef ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://github.com/luci/luci-go/commit/4bfe11ebfbb203db86104241a7a748df16f507ef
Message was sent while issue was closed.
https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go File milo/common/pubsub.go (right): https://codereview.chromium.org/2981683002/diff/120001/milo/common/pubsub.go#... milo/common/pubsub.go:57: type pubsubClientFactory interface { On 2017/07/14 17:41:49, Ryan Tseng wrote: > On 2017/07/14 01:09:04, nodir wrote: > > wouldn't > > > > type pubsubClientFactory func(ctx context.Context, projectID string) > > (pubsubClient, error) > > > > > > be simpler? You wouldn't need prodPubSubClientFactory as a struct > > Thanks, I forgot this was legal > > edit: this is possible but there is a trap (I learned today), Bound methods will > not match this type interface, but a closed function will. I made a comment > about that in the test code. I am not following. The following compiles: https://play.golang.org/p/AFZ8owXhzY
Message was sent while issue was closed.
What if you stuck it in an interface and back out again? https://play.golang.org/p/QveWZ1MQ1s
Message was sent while issue was closed.
On 2017/07/14 18:26:56, Ryan Tseng wrote: > What if you stuck it in an interface and back out again? > https://play.golang.org/p/QveWZ1MQ1s https://play.golang.org/p/0guz4uoECu
Message was sent while issue was closed.
I see, so a generic function conforming to a function type cannot be cased into the specific function type, but it can be casted to an interface matching the function type and then assigned to a variable of that function type. thanks for the clarification. |
