Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for more generic resolver #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

spkrka
Copy link
Member

@spkrka spkrka commented Feb 12, 2019

No description provided.

*/
public MemcacheClientBuilder<V> withSrvResolver(final DnsSrvResolver srvResolver) {
this.srvResolver = checkNotNull(srvResolver, "srvResolver");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I screwed with this one, but this change is no longer needed

client = clients.get(0);
}
}
final SrvKetamaClient client =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should not be named SrvKetamaClient now

new SrvKetamaClient(
resolver,
DEFAULT_SCHEDULED_EXECUTOR.get(),
dnsRefreshPeriod,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be renamed

import com.spotify.folsom.ConnectionChangeListener;
import com.spotify.folsom.ObservableClient;
import com.spotify.folsom.RawMemcacheClient;
import com.spotify.folsom.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to use wildcard imports?

for (LookupResult lookupResult : lookupResults) {
hosts.add(HostAndPort.fromParts(lookupResult.host(), lookupResult.port()));
ttl = Math.min(ttl, lookupResult.ttl());
List<ResolveResult> resolveResults = resolver.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be final

hosts.add(HostAndPort.fromParts(lookupResult.host(), lookupResult.port()));
ttl = Math.min(ttl, lookupResult.ttl());
List<ResolveResult> resolveResults = resolver.resolve();
List<HostAndPort> hosts = Lists.newArrayListWithCapacity(resolveResults.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be final

@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling bbb282d on krka/resolver2 into 57e7745 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants