Support for watching SRV records and getting update notifications

[rouzwawi]

PR for #8

Features

• Give a mechanism for getting notified when the set of SRV records for a fqdn change
• Make it possible to use derivative values of LookupResult as the current set, with notifications only happening if that derived set changes

API

interface DnsSrvWatcher<T> extends Closeable {
  ChangeNotifier<T> watch(String fqdn);
}

interface ChangeNotifier<T> {
  Set<T> current();
  void setListener(Listener<T> listener, boolean fire);
  void close();

  interface Listener<T> {
    void endpointsChanged(ChangeNotification<T> changeNotification);
  }

  interface ChangeNotification<T> {
    Set<T> current();
    Set<T> previous();
  }
}

And the construction API:

DnsSrvResolver resolver = DnsSrvResolvers.newBuilder()
    // configure
    .build();

DnsSrvWatcher<String> watcher =
    DnsSrvResolvers.newWatcherBuilder(resolver, (lookup) -> lookupToString(lookup))
        .polling(1, TimeUnit.SECONDS)
        .usingExecutor(myScheduledExecutorService)
        .withErrorHandler(myErrorHandler)
        .build();

[pettermahlen]

Should this be final? I'm wondering if it's protected in order to be overridable (doesn't feel like it) or callable by a sub-class or both. Ditto for newChangeNotification below.

[pettermahlen]

Looks good - I think the term 'endpoints' in quite a few places should be replaced with 'records' or something along those lines.

[spkrka]

I'd replace all of this custom notification code with Guava EventBus.

[rouzwawi]

Just tried it out and it only get's marginally simpler: https://gist.github.com/rouzwawi/fc246c40fb176cd3e792

It could be worth it given that it makes dispatch to several listeners a bit easier.

[pettermahlen]

I'm not sure using EventBus makes sense to me, from reading https://code.google.com/p/guava-libraries/wiki/EventBusExplained. It seems a little like overkill.

If we do want to use it, I think the gist needs to be made a little more complex: either make the API support multiple listeners, or revert to using an AtomicReference for the single listener.

[spkrka]

Yes, and I think setListener should be replaced with addListener. And I don't think AbstractChangeNotifier needs to keep track of the listener. Actually, shouldn't there be one shared eventbus per srv poller? Maybe I need to read the code more closely.

I am also slightly worried about ordering with the initial notification (fire=true). You could have a race condition between an actual event and the initial faked one.

[spkrka]

petter: yes, probably a bit overkill if we only want a single listener. shouldn't multiple listeners be able to listen on the same dns updates though? but perhaps that's not a common usecase.

[pettermahlen]

Re: race between notifications, I agree. This is not something that is expected to be updating with super-high frequency, so it should be quite OK to make setListener and fireRecordsUpdated synchronized, I think?

[spkrka]

Overall it seems overly complex - can we make it simpler with fewer interfaces and factories without losing functionality?

[rouzwawi]

@spkrka I understand that it looks complex when looking into the details. But the complexity is only ever exposed if someone starts using DnsSrvWatcherBuilder.customTrigger().

If only used with the polling configuration, the exposed api surface is actually pretty simple IMO.

[spkrka]

synchronize around listener set and fire, to avoid race conditions

[rouzwawi]

Fixed the race issues with listener firing (I hope), and fixed commented tweaks.

The comment on propagating lookup/transformer errors to the listener is still open. I'm unsure if that's the right thing to do. The listener should not have to know about errors, just changes. Because of the async nature of the whole thing, I'm thinking it's good enough to just log the errors and be done with it.

[spkrka]

I think it depends on the type of error. Is it a temporary network problem? Logging is fine.

Is the transform function throwing errors? The user of the library should probably be notified.

[rouzwawi]

How do you suggest that would happen?

The user supplied function is used by the library, and if it throws an exception, the error is logged.

[spkrka]

Error notification could be part of the change listener callback

[rouzwawi]

Things that can go wrong when the notifiers poll for changes:

• resolver.resolve(fqdn) throws a DnsException which is a RuntimeException
• resultTransformer.apply(node) throws an exception
• resultTransformer.apply(node) return null
• listener.onChange(changeNotification) throws exception

The two transformer related cases are programmer errors. So just failing fast and printing it in the logs should be good enough. There's no good reaction the code can have to such things happening.

The first, resolver error might be good to catch in order to take action when dns lookups are flaky.

The last one should also be a programmer error since listeners should generally never throw.

Sounds reasonable?

[pettermahlen]

ImmutableList.copyOf() throws NPE if the collection to be copied is null, so no need to use checkNotNull here.

[pettermahlen]

LGTM, fix the remaining issues if you like, then merge.