Provide a new means to update a Eureka application record periodically

[seh]

Define the ScheduleAppUpdates and NewAppSource methods on EurekaConnection. The former supplies a channel of periodic updates to an application, while the latter holds the latest successfully acquired update of an application and makes it available for reading safely from any goroutine.

This addresses #36, but for now I left EurekaConnection's UpdateApp method in place, for fear of breaking existing callers. I considered documenting it as "deprecated", but without a commitment to remove it by a given date it felt disingenuous to wag a finger like that.

[seh]

Also, while I did include a set of example calls to these new methods, I did not include tests. I admit that I am unsure how best to test these methods, because I can't rely on having a particular Eureka server available.

I considered writing a fake EurekaConnection, but since it's not an interface, it's hard to substitute such a fake in a unit test. If you have suggestions as to how to write such a test (in this case confirming that we can receive periodic updates to a given application record), I'm willing to do so.

And now that I think about it more, if these new methods were free functions instead of being methods on EurekaConnection, they could instead accept a parameter like the following:

type ApplicationSupplier interface {
    getApp(string) (*Application, error)
}

The struct EurekaConnection already implements that interface.

Let me know what you think.

[tysonstewart]

@seh Thank you so much for your contribution! We'll look over it and try to get it merged as soon as possible. Just wanted to make sure you knew we're listening. :-)

[tysonstewart]

Should this be either u.Err == nil or u.App != nil (or both)?

[seh]

There's an inconsistency with the documentation for Latest at line 183, which says that if the most recent update attempt fails, we do want to assign nil to u.App here—and later return nil from Latest. Here, though, since we ignore failed updates—where an error arose—we won't wind up assigning nil to s.app unless GetApp can return a nil Application pointer with no corresponding error. My reading of GetApp says that it won't return a nil Application pointer when the corresponding error is nil, meaning that we'd only ever assign a non-nil Application pointer to s.app here.

Given that, I can see two choices:

• Remove this guard condition here, and always take the latest Application pointer, even if it's nil, making it consistent with the documentation for Latest.
• Guard on u.App not being nil here, as you suggest, and amend the documentation for Latest to indicate that once Latest returns non-nil, it will never return nil again. Unfortunately, it can still return nil if no update attempts have succeeded yet.

I can see callers wanting a kind of self-preservation here, where if we had a good Application in hand at time T1, and later at time T2 we fail to update it, and at T3 we ask for the latest Application, we might like to have the "last known good" one still available to us. We could make this an option on NewAppSource—a boolean preserveLastGood parameter.

I'd appreciate your feedback.

[itsrainy]

Between your two choices, I think I'd favor the first one. If the most recent update attempt wasn't successful it doesn't seem like we should try to hide that from the caller.

As far as the self-preservation idea, I can definitely see it being useful but I'm a bit torn. It seems like something that might be use-case specific and should be implemented on the caller-side.

[tysonstewart]

I agree with @jcox92; while I definitely see the value of a self-preservation option, as a library, I think it's better left up to the implementor to decide for him- or herself that it's desired functionality. I vote option 1.

Thanks for the great analysis. 👍

[seh]

I changed NewAppSource to always take the latest Application update, whether it succeeded or failed to provide a non-nil pointer.

[seh]

And yet reading the code today, I see that that intended change to NewAppSource never made it in.
I'm adjusting it now to align properly with the VIP address monitoring that I'm adding for #53.

[itsrainy]

And now that I think about it more, if these new methods were free functions instead of being methods on EurekaConnection, they could instead accept a parameter like the following:

type ApplicationSupplier interface {
    getApp(string) (*Application, error)
}

The struct EurekaConnection already implements that interface.

I think that's a good way to make these more testable. Although instead of making them free functions, you could make them methods on the ApplicationSupplier interface. That might make it cleaner from the caller's side.

[itsrainy]

Blank line unnecessary

[seh]

I've seen that in the standard library, the style is to separate documentation paragraphs with blank lines. Are you suggesting that the sentence that follows the blank line should be part of the preceding paragraph?

[itsrainy]

Ah okay. I didn't realize you intended to make it a new paragraph. 👍

[seh]

Although instead of making them free functions, you could make them methods on the ApplicationSupplier interface. That might make it cleaner from the caller's side.

Well, I noticed when trying to introduce this interface is that ScheduleAppUpdates requires access to EurekaConnection's PollInterval field. That means that having it rely solely on a GetApp method is insufficient. I could have ScheduleAppUpdates demand a polling interval parameter, but that dilutes the utility of EurekaConnection's PollInterval field. As far as I can tell, this is the whole point of including that field to begin with.

I am now less certain of the utility of introducing the proposed ApplicationSupplier interface.

// ApplicationSupplier is the interface that wraps the GetApp method, allowing
// retrieval of registered Eureka applications.
//
// GetApp returns the Eureka application with the given name. If no such
// application exists, it returns AppNotFoundError.
type ApplicationSupplier interface {
    GetApp(name string) (*Application, error)
}

Instead, the best I can see doing is hoisting ScheduleAppUpdates up to an interface, and having NewAppSource rely on that interface.

type ApplicationUpdater interface {
    ScheduleAppUpdates(name string, prime bool, done <-chan struct{}) <-chan AppUpdate {
}

Then:

func NewAppSource(u ApplicationUpdater, name string, prime bool) *AppSource {
// ...

That makes NewAppSource testable with a mock ApplicationUpdater, but we still can't test ScheduleAppUpdates with a mock of the GetApp method, which was my real goal.

[seh]

I'd appreciate any further critique you can offer here.

[itsrainy]

Sorry for letting this sit so long @seh. I missed your most recent commit. Other than tests, I think you've addressed all of my concerns. It'd be nice to be able to write tests, but I think mocking out the EurekaSupplier interface is outside of the scope of what you're trying to accomplish here.

I'm good with getting this merged in. @tysonstewart or @ryansb any additional thoughts?

[ryansb]

How would you feel about calling the prime parameter await instead?

[seh]

That's a fine suggestion. I'll make the change shortly.

[seh]

I made the change to the NewAppSource method, forgetting to change this one too. I'll do that now.

[ryansb]

Looks good to me, just the minor nit on the prime parameter.