Replace ReadonlyTargets interface with a TargetsConfig channel #815
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ian-mi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ian-mi
force-pushed
the
target-config-channel
branch
from
0a5f861 to
8da553f
Compare
|
I think this is probably not the best time to do this refactoring unless it provides significant value. Grant is working the controller which also uses the lib for writing data. @grantr |
A channel provides a simple and safe interface for config updates. The in-memory config is no longer needed as tests can simply write config values to the config channel. For the volume config the config channel replaces the notify channel and the config watcher writes updated config values to the channel.
ian-mi
force-pushed
the
target-config-channel
branch
from
8da553f to
07aeb1a
Compare
ian-mi
changed the title
|
/hold wait for broker reconcile to be merged |
|
Got time to take a more detailed look. Thanks for sharing ways to improve this lib and adopt best practices! We should discuss what benefit this change could bring once the reconciler PR is merged. Here are a couple of things that might require some consideration:
|
|
Thanks for taking a look! The benefit of this change would be merely the simplification of concurrent communication and adherence to idiomatic go best practices.
|
Only create a goroutine when there is concurrent work to do. Use a buffered channel to bound concurrency instead of creating a fixed number of workers. Pass the targets config to workers when they are created rather than synchronizing shared access to config.
TargetsWatcher uses a channel to indicate the available of an updated TargetsConfig which is returned alongside each config read.
handler will now not attempt to pull a message until it has acquired a concurrency token. Messages are pulled in a separate goroutine in order to avoid blocking handle. New messages are passed back to the goroutine so that they can be handled using the latest broker config.
This allows SyncPool to use a plain map since all access is synchronized through its goroutine.
|
I believe this PR now addresses 2-4. For 1 I can simply re-add the in-memory config code, I think it makes sense for the in-memory config store used by the controller to be completely separate from the config broadcast functionality used by the data plane. |
This is necessary since the test introspects internal state.
|
The following is the coverage report on the affected files.
|
|
@ian-mi: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Thanks for the updates! My thoughts:
It could be debatable if this change is a pattern overuse (sharing state via communication). There is code that only reads the config data (config-reader, e.g. handler, processors) and there is code that needs to know when the config is updated (config-notifiee, e.g. syncpool). This change inevitably blurs the boundary between config-reader and config-notifiee - config-reader now has to actively manage the update signal, which would make the config-reader's code more complicated and harder to follow. I believe config-reader should see config as "static" data and shouldn't worry about signals at all. FWIW, for some existing config lib I know (viper, micro-go/config), I don't see them forcing config-readers to accept config data from channel (as signal).
My thoughts aside, I'm very likely biased and can't see the benefits from this change (e.g. if this change could provide a significant performance improvement). So better to have more eyes on this (@grantr @Harwayne @grac3gao @ericlem, feel free to share your thoughts). I will put a temp hold on this since the broker controller code hasn't yet been merged.
/hold
| } | ||
|
|
||
| // Start starts the handler. | ||
| // done func will be called if the pubsub inbound is closed. | ||
| func (h *Handler) Start(ctx context.Context, done func(error)) { | ||
| func (h *Handler) Start(ctx context.Context, config *config.Broker) { |
There was a problem hiding this comment.
I intentionally avoided specific parameters (e.g. config.Broker). Handler <-> broker is not necessarily 1-1 mapping. For retry, it would be one handler per target (trigger). In my thinking, the handler's only responsibility is to receive messages and invoke processors. It shouldn't make any assumption of what data is needed for processors. The handler creator needs to put the data in the context.
There was a problem hiding this comment.
Currently, there is a 1-1 correspondence between brokers and handlers, and context is used as a form of implicit argument passing from handlers to processors and as well as between processors. The context docs warn against this:
Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.
I think in this case if we need a handler for a different purpose, we should simply write a new handler rather than trying to write an all-purpose handler as this level of abstraction is not well suited for go.
There was a problem hiding this comment.
I see it as a suggestion rather than requirement. Knative code in a lot of places doesn't follow this suggestion - client/informer injection, logger in the context, etc.
There was a problem hiding this comment.
Knative is definitely guilty of abusing context as well, but I don't necessarily think we should copy that practice if we can avoid it.
| } else { | ||
| go h.handleMessage(handlerctx.WithBroker(ctx, config), r.msg, tokens) | ||
| } | ||
| case config = <-h.ConfigCh: |
There was a problem hiding this comment.
My $0.02 here: I feel this actually made code harder to read because the handler now has to support config updates
There was a problem hiding this comment.
I think it is a matter of getting used to the message passing style of communication. Maybe it would be useful to have a wider discussion on whether we should prefer this style of concurrency in general.
There was a problem hiding this comment.
My point was not about whether people can read this style. It was about whether it's necessary. Like I mentioned in the other comment, I see handler as a config-reader and it doesn't have to act on config updates, which also happens to make code simpler.
I have @ the folks who are actively working on broker to share their opinions.
| case <-targets.Updated: | ||
| targets = p.targetsWatcher.Targets() |
There was a problem hiding this comment.
This part confuses me. This looks like the same "signal" idea as we now have? Except you wrap them into a struct.
There was a problem hiding this comment.
It's slightly different, this signal only indicates that the current config is out of date and it can broadcast to any number of listeners.
The old signal channel required every signal to pulled downstream or the watcher would block and it also did not support broadcast. It also did not tie the signal to the config, so if the config was updated multiple times before a listener had a chance to sync the listener would still receive multiple signals and sync more than necessary.
With TargetsWatcher, a listener receives the latest config along with an update signal tied to that specific config value. The listener can use this signal to determine when that config value should be updated, but is not required to receive from that channel and the watcher will not be blocked.
There was a problem hiding this comment.
I guess I'm fine with TargetsWatcher if it provides improvements like you mentioned here. But I'd like the syncpool being the only one handles update signal. Not handler, not processors.
| if _, ok := p.targets.GetBrokerByKey(key.(string)); !ok { | ||
| value.(*handler.Handler).Stop() | ||
| p.pool.Delete(key) | ||
| for key, handler := range p.pool { |
There was a problem hiding this comment.
The idea of readonly targets is (although not 100% safe) to make it more obvious that the targets are readonly and more difficult to modify targets.
There was a problem hiding this comment.
I see, maybe I should add the interface back to improve safety? I'm typically a fan of using proto directly but I'm also used to a language where protos are immutable. Perhaps immutable by convention is good enough? My concern with using an interface is that it has to be kept up-to-date with the proto.
There was a problem hiding this comment.
@ian-mi what's your specific concern with the interface needing to be kept up-to-date? Are you concerned about runtime errors? Engineering time used keeping the interface current?
|
I took a look at the other config systems that you mentioned. viper is not thread-safe and would require a process to serialize access to config: spf13/viper#268. On the other hand go-micro/config does include a config Watcher type that can be used to watch for configuration changes so I think its design is more comparable to TargetsWatcher. One aspect of TargetsWatcher that needs clarification is that it does not require listening on the update channel. Targets() can always be called get the latest config value. The update channel is provided as a means of avoiding spurious updates when state needs to be synchronized based on config updates as is the case with SyncPool. |
I was wondering how By mentioning those config libs, my point was that we need treat config-reader (who just wants to read the latest data) and config-notifiee (who needs to act on config updates) differently. We shouldn't force config-reader to handle update signals. But again, that's just my opinion. |
|
@ian-mi: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
I haven't had a chance to look at this yet but hope to soon. Let's try to resolve it for v0.15.0-M1 sprint. |
There was a problem hiding this comment.
@ian-mi I feel like I don't have enough context to understand what the practical improvement is here. Can you provide some examples of how one would use this new interface to implement a handler or processor, and why that makes reading or maintaining the code easier?
From the broker controller perspective, there is one goroutine that reads the current state, modifies, and writes back. The broker controller must manage this concurrency itself because it needs to serialize writes to the configmap and the memory targets impl doesn't do this (but #830 would help).
I expect this to be a temporary condition because once we adopt #852, the read-modify-write code won't need to deal with concurrency because only one thread will ever be operating on the targets config and configmap.
| if _, ok := p.targets.GetBrokerByKey(key.(string)); !ok { | ||
| value.(*handler.Handler).Stop() | ||
| p.pool.Delete(key) | ||
| for key, handler := range p.pool { |
There was a problem hiding this comment.
@ian-mi what's your specific concern with the interface needing to be kept up-to-date? Are you concerned about runtime errors? Engineering time used keeping the interface current?
|
Closing since I haven't had time to update this PR. |
A channel provides a simple and safe interface for config updates. The in-memory
config is no longer needed as tests can simply write config values to the config
channel. For the volume config the config channel replaces the notify channel
and the config watcher writes updated config values to the channel.
Proposed Changes