Name of getHostnamesInfo
is misleading a little bit. It returns to listOfHostnames
, so calling it getHostnames
or getHostnamesForUser
would be less confusing.
First, to DRY the code, I'd try to extract out some hostname list handling logic to a separate class:
public class Hosts {
private final LinkedList<String> hostsnames = newLinkedList();
public Hosts(final List<String> hosts) {
checkNotNull(hosts, "hosts cannot be null");
this.hostsnames.addAll(hosts);
}
public Optional<String> getNextAvailableHostname() {
while (!hostsnames.isEmpty()) {
String firstHostname = hostsnames.removeFirst();
if (!ClientUtils.isEmpty(firstHostname) && !ShardMapping.isBlockHost(firstHostname)) {
return Optional.of(firstHostname);
}
}
return Optional.absent();
}
public boolean isEmpty() {
return hostsnames.isEmpty();
}
}
It makes getData
simpler:
@Override
public ListenableFuture<DataResponse> getData(final DataKey key) {
// given a userId, find all the hostnames
// so it can also have four hostname or one hostname or six hostname as
// well in the list
final LinkedList<String> hostnames = getHostnames(key.getUserId());
final Hosts hosts = new Hosts(hostnames);
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (!nextAvailableHost.isPresent()) {
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
return responseFuture;
}
checkState(nextAvailableHost.isPresent());
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
return responseFuture;
}
and improves a little bit the RetryCallback.onFailure
too:
@Override
public void onFailure(final Throwable ex) {
if (ex instanceof SocketException) {
// if it comes here, then it means some of the servers are down so adding it into block list
ShardMapping.blockHost(hostname);
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
return;
}
// either all the servers are down or all the servers were in block list
if (hosts.isEmpty()) {
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
}
} else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
DataStatusEnum.ERROR));
}
// is there any other error I should look into?
}
You might notice some other duplication here, especially if you invert the condition in getData
:
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
} else {
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
}
return responseFuture;
I have created a temporary execute
method for it in the Hosts
class:
public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key, final DataFetcher dataFetcher) {
final Optional<String> nextAvailableHost = getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), this);
return;
}
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
}
It probably has a bad name and probably it should not be in the Hosts
class but it's enough in this internal step.
Now getData
looks like this:
@Override
public ListenableFuture<DataResponse> getData(final DataKey key) {
final LinkedList<String> hostnames = getHostnames(key.getUserId());
final Hosts hosts = new Hosts(hostnames);
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
hosts.execute(responseFuture, key, this);
return responseFuture;
}
And onFailure
:
@Override
public void onFailure(final Throwable ex) {
if (ex instanceof SocketException) {
// if it comes here, then it means some of the servers are down so adding it into block list
ShardMapping.blockHost(hostname);
hosts.execute(responseFuture, key, dataFetcher);
} else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
DataStatusEnum.ERROR));
}
}
I don't feel that Hosts
class fulfills the single responsibility principle. It handles hostnames well but the execute
method seems another responsibility. Furthermore, it uses only the public API of Hosts
which does not suggest high cohesion. So I moved this method to a new class:
public class Retryer {
public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key,
final DataFetcher dataFetcher, final Hosts hosts) {
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
return;
}
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
}
}
Usage in onFailure
:
new Retryer().execute(responseFuture, key, dataFetcher, hosts);
and in getData
:
new Retryer().execute(responseFuture, key, this, hosts);
Note that isEmpty
in Hosts
is not used, you could remove it. Furthermore, the Retryer
name does not looks good, it smells for me.
Another duplication is in this line:
responseFuture.set(new DataResponse(httpException.getResponseBodyAsString(), error,
DataStatusEnum.ERROR));
and this one:
responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR));
I moved these calls also to the Retryer
class:
public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key,
final DataFetcher dataFetcher, final Hosts hosts) {
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
dataFetcher.executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
return;
}
setServiceUnavailable(responseFuture);
}
private void setServiceUnavailable(final SettableFuture<DataResponse> responseFuture) {
final String errorMessage = null;
final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
setErrorReponse(responseFuture, errorMessage, error);
}
public void setErrorReponse(final SettableFuture<DataResponse> responseFuture, final String errorMessage,
final DataErrorEnum error) {
final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
responseFuture.set(dataResponse);
}
I would consider creating a new constructor in the DataResponse
which omits the errorMessage
parameter so passing null
does not clutter the code. (When I see a null parameter I always wonder which parameter is it. Unfortunately I have to check the implementation to figure it out.)
Another thing is that errorMessage
can be null
. Setting it to an empty string might save you from a few NullPointerException
s later (if that's possible and sane).
Let's go back to the ugly Retryer
. Currently it's the only one caller of executeForServers
. Additionally, I have a feeling that I would prefer to call it DataFetcherExecutor
or DataFetcher
which is even better. Now, I changed DataFetcher
from interface to class and moved both Retryer.execute
and DataClient.executeForServers
into it (along with their dependencies). When I've finished I got this:
public class DataFetcher {
private final AsyncRestTemplate restTemplate = new AsyncRestTemplate(
new HttpComponentsAsyncClientHttpRequestFactory());
public void execute(final SettableFuture<DataResponse> responseFuture, final DataKey key, final Hosts hosts) {
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
executeForServers(responseFuture, key, nextAvailableHost.get(), hosts);
return;
}
setServiceUnavailable(responseFuture);
}
public void executeForServers(final SettableFuture<DataResponse> responseFuture, final DataKey key,
final String hostname, final Hosts hosts) {
final ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this, responseFuture,
hostname, key, hosts);
restTemplate.exchange(generateURL(hostname, key), HttpMethod.GET, key.getEntity(), String.class).addCallback(
callback);
}
private void setServiceUnavailable(final SettableFuture<DataResponse> responseFuture) {
final String errorMessage = null;
final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
setErrorReponse(responseFuture, errorMessage, error);
}
public void setErrorReponse(final SettableFuture<DataResponse> responseFuture, final String errorMessage,
final DataErrorEnum error) {
final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
responseFuture.set(dataResponse);
}
... // generateURL is here
}
Retryer
is empty, it can be removed now. DataClient
also a lot simpler, it does not implement DataFetcher
any more and it contains only some simple wiring logic:
public class DataClient implements Client {
@Override
public ListenableFuture<DataResponse> getData(final DataKey key) {
final LinkedList<String> hostnames = getHostnames(key.getUserId());
final Hosts hosts = new Hosts(hostnames);
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
final DataFetcher dataFetcher = new DataFetcher();
dataFetcher.execute(responseFuture, key, hosts);
return responseFuture;
}
... // getHostnames is here
}
And RetryCallback
:
public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
private final SettableFuture<DataResponse> responseFuture;
private final String hostname;
private final DataKey key;
private final DataFetcher dataFetcher;
private final Hosts hosts;
public RetryCallback(final DataFetcher dataFetcher, final SettableFuture<DataResponse> responseFuture,
final String hostname, final DataKey key, final Hosts hosts) {
this.dataFetcher = checkNotNull(dataFetcher);
this.responseFuture = checkNotNull(responseFuture);
this.hostname = checkNotNull(hostname);
this.key = checkNotNull(key);
this.hosts = checkNotNull(hosts);
}
@Override
public void onSuccess(ResponseEntity<String> result) {
responseFuture.set(new DataResponse(result.getBody(), DataErrorEnum.OK,
DataStatusEnum.SUCCESS));
}
@Override
public void onFailure(final Throwable ex) {
if (ex instanceof SocketException) {
// if it comes here, then it means some of the servers are down so adding it into block list
ShardMapping.blockHost(hostname);
dataFetcher.execute(responseFuture, key, hosts);
} else { // this is for 4xx (HttpClientErrorException) and 5xx (HttpServerErrorException) error coming from server side
HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
final String errorMessage = httpException.getResponseBodyAsString();
dataFetcher.setErrorReponse(responseFuture, errorMessage, error);
}
}
}
For me it seems quite good and simple, but do not stop yet.
Let's try to move body of onSuccess
to DataFetcher
as well. DataFetches
contains some similar code already.
public void setSuccessResponse(final SettableFuture<DataResponse> responseFuture, final String message) {
final DataResponse dataResponse = new DataResponse(message, DataErrorEnum.OK, DataStatusEnum.SUCCESS);
responseFuture.set(dataResponse);
}
Usage in RetryCallback
:
@Override
public void onSuccess(ResponseEntity<String> result) {
final String message = result.getBody();
dataFetcher.setSuccessResponse(responseFuture, message);
}
Notice in RetryCallback
that dataFetcher
is the only one object which uses the responseFuture
reference of RetryCallback
. It might be in the DataFetcher
class and probably RetryCallback
don't have to know anything about responseFuture
, it should be an internal detail of DataFetcher
. Let's try that and the same with the hosts
and key
fields too.
DataFetcher
with the new fields and constructor parameters:
public class DataFetcher {
private final AsyncRestTemplate restTemplate = new AsyncRestTemplate(
new HttpComponentsAsyncClientHttpRequestFactory());
private final SettableFuture<DataResponse> responseFuture;
private final DataKey key;
private final Hosts hosts;
public DataFetcher(final SettableFuture<DataResponse> responseFuture, final DataKey key, final Hosts hosts) {
this.responseFuture = checkNotNull(responseFuture);
this.key = checkNotNull(key);
this.hosts = checkNotNull(hosts);
}
public void tryNextHost() {
final Optional<String> nextAvailableHost = hosts.getNextAvailableHostname();
if (nextAvailableHost.isPresent()) {
doExchange(nextAvailableHost.get());
return;
}
setServiceUnavailable();
}
private void doExchange(final String hostname) {
final ListenableFutureCallback<ResponseEntity<String>> callback = new RetryCallback(this, hostname);
final URI url = generateURL(hostname, key);
restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class).addCallback(
callback);
}
private void setServiceUnavailable() {
final String errorMessage = null;
final DataErrorEnum error = DataErrorEnum.SERVICE_UNAVAILABLE;
setErrorReponse(errorMessage, error);
}
public void setErrorReponse(final String errorMessage, final DataErrorEnum error) {
final DataResponse dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
responseFuture.set(dataResponse);
}
public void setSuccessResponse(final String message) {
final DataResponse dataResponse = new DataResponse(message, DataErrorEnum.OK, DataStatusEnum.SUCCESS);
responseFuture.set(dataResponse);
}
...
}
Note the removed responseFuture
parameters of almost every method. I've also renamed a few methods.
Now, here is RetryCallback
:
public class RetryCallback implements ListenableFutureCallback<ResponseEntity<String>> {
private final String hostname;
private final DataFetcher dataFetcher;
public RetryCallback(final DataFetcher dataFetcher, final String hostname) {
this.dataFetcher = checkNotNull(dataFetcher);
this.hostname = checkNotNull(hostname);
}
@Override
public void onSuccess(final ResponseEntity<String> result) {
final String message = result.getBody();
dataFetcher.setSuccessResponse(message);
}
@Override
public void onFailure(final Throwable ex) {
if (ex instanceof SocketException) {
// if it comes here, then it means some of the servers are down so
// adding it into block list
ShardMapping.blockHost(hostname);
dataFetcher.tryNextHost();
} else { // this is for 4xx (HttpClientErrorException) and 5xx
// (HttpServerErrorException) error coming from server side
final HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
final DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
final String errorMessage = httpException.getResponseBodyAsString();
dataFetcher.setErrorReponse(errorMessage, error);
}
}
}
It is so much simpler now.
Finally, DataClient
:
public class DataClient implements Client {
@Override
public ListenableFuture<DataResponse> getData(final DataKey key) {
final LinkedList<String> hostnames = getHostnames(key.getUserId());
final Hosts hosts = new Hosts(hostnames);
final SettableFuture<DataResponse> responseFuture = SettableFuture.create();
final DataFetcher dataFetcher = new DataFetcher(responseFuture, key, hosts);
dataFetcher.tryNextHost();
return responseFuture;
}
...
}
It seems quite good but you should test it: write unit tests for the refactored classes. If it's easy it might sign that it's a good design. If it's hard - that means that the code definitely needs further improvements. (Killing the Helper class, part two will help you.)
Finally, I would improve RetryCallback
a little:
if (ex instanceof SocketException) {
// if it comes here, then it means some of the servers are down so
// adding it into block list
ShardMapping.blockHost(hostname);
dataFetcher.tryNextHost();
} else {
...
}
You could extract out an explanatory variable:
final boolean serverDown = ex instanceof SocketException;
if (serverDown) {
ShardMapping.blockHost(hostname);
dataFetcher.tryNextHost();
} else {
It eliminates the comment. See: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler; Clean Code by Robert C. Martin, G19: Use Explanatory Variables.
onFailure
(initially, there's no failure, but the situation is similar: the remaining hosts are to be tried). \$\endgroup\$listOfHostnames
(Make it to anIterator<String>
returning a next non-blocked host? Make it return aFuture
?). This asynchronicity drives me crazy, \$\endgroup\$