3
\$\begingroup\$

I'm consuming some messages from a rabbit queue. Within the consumer/processor I need to invalidate a varnish cache and requeue the message if the purge failed. I'm using a local purge proxy. The consumer is a Dropwizard microservice but I'm using a BasicHttpClient because JerseyClient doesn't allow me (AFAIK) to set PURGE as http verb.

I'm trying to improve my error handling.. Is throwing an IllegalStateException fine or should I improve it? Thanks

public void invalidateVarnishCache( String level, String idUrl ) throws IOException {
    try{
        String xPurgeRegex = level+"/"+idUrl+"$";
        Header header = new BasicHeader( "X-Purge-Regex", xPurgeRegex );
        BasicHttpRequest purgeRequest = new BasicHttpRequest("PURGE", "/" );
        purgeRequest.setHeader(header);

        LOGGER.log( LogEntries.VarnPurge, level, idUrl, host, xPurgeRegex ); 

        response = httpClient.execute(host, purgeRequest);

        entity = response.getEntity();

        int statusCode = response.getStatusLine().getStatusCode();

        if( !is2XXResponse( statusCode ) ){

            int respLength = entity.getContent().available();
            byte[] errorResp = new byte[ respLength ];
            entity.getContent().read(errorResp, 0, respLength);

            LOGGER.log( LogEntries.VarnPurgeErrResp, statusCode, new String(errorResp) ); 

            throw new IllegalStateException("Varnish local proxy returned response with status code "+statusCode+" from Varnish backend. Consumed message will be requeued.");
                      /////////////////////
        }else{
            LOGGER.log( LogEntries.VarnPurgeSuccessResp, statusCode ); 
        }

        EntityUtils.consume(entity);

    }catch( IOException ex ){
        LOGGER.log( LogEntries.VarnPurgeException, ex ); 
        throw ex;
    }
}

This is the consumer..

public MessageResult apply(UpdateMessage t) {

    if( !agentValidator.validMessage( t ) ){
        return MessageResult.ACK; // discard message
    }

    String urlRequest = t.getLink();

    String[] splitted = urlRequest.split( "/", -1 );
    String level = splitted[2];
    String idUrl = splitted[3];

    try{
        // Varnish cache purge. It will requeue if purge fails for any reason.
        try{
            pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
        }catch(Exception e){

            LOGGER.log( LogEntries.PurgeError, host, urlRequest, e );
            return MessageResult.NACK_REQUEUE;
        }

        // retrieve resource from ohter microservice
        try{
            response = connector.getLatestStateOfResource( urlRequest );    
        }catch(Exception e){

            LOGGER.log( LogEntries.DBServiceError, pcsHost, urlRequest, e );                                                    
            return MessageResult.NACK_REQUEUE;
        }

        // .....
        // more logic
        // ....

    }catch(Exception e){

        LOGGER.log( LogEntries.MsgProcessingError, t.toString(), e );
        return MessageResult.NACK_REQUEUE;
    }

    LOGGER.log( LogEntries.MsgConsumedSuccess, t.toString() );

    return MessageResult.ACK; // SUCCESS. Remove message from queue.

}
\$\endgroup\$
2
\$\begingroup\$

The Consumer

The nested try-catch blocks look weird and useless. Since purgeVarnishBeforePcsRequest and getLatestStateOfResource throw exceptions, they may be caught in the outer catch block.

try {
  pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
  response = connector.getLatestStateOfResource( urlRequest );
  // .....
  return MessageResult.ACK;
} catch (Exception e) {
  // logger
  return MessageResult.NACK_REQUEUE;
}

By the way, catching of Exception type might be a bad coding practice, but I'd not discuss it here because the context is not enough to judge.

invalidateVarnishCache

The only purpose of the try-catch block in the original code is to trace the IOException using a hypothetical commented logger and then re-throw it. I doubt that the block itself is useful, because exactly the same exception is re-thrown and must be handled somewhere among the callers and even probably also logged by them. Wouldn't it be redundant?

Concerning the IllegalStateException, the problem is that it is a RuntimeException and the caller can miss to handle it properly, with hardly predictable consequences.

I don't know what is the range of response statuses that this sort of proxy can return, but your implementation does not make distinction between 4xx and 5xx codes, which may lead either to a client-side error (your request is invalid, 4xx) or to a server-side error (5xx), which is semantically close to an I/O exception. For the latter, I can suggest to wrap the response message into a IOexception and re-throw it. For the former, it's still up to you to decide how to handle it, probably just logging is enough.

\$\endgroup\$
  • \$\begingroup\$ Well I chose to use specific try catch block for single important parts in order to have better logging. The big try catch is for unexpected exceptions in any of the steps. For instance even though I have many tests in place I don't want an unexpected null pointer exception to crash the microservice but just log it with severe tag (and someone will be notified via email). \$\endgroup\$ – Ga Sacchi Apr 26 '16 at 22:18
  • \$\begingroup\$ Thanks for your suggestions. I've updated with the missing logs.. \$\endgroup\$ – Ga Sacchi Apr 26 '16 at 22:30
  • \$\begingroup\$ For the former, it's still up to you to decide how to handle it, probably just logging is enough .. well if any kind of failure happen requirement is that I requeue it.. \$\endgroup\$ – Ga Sacchi Apr 26 '16 at 22:33
  • \$\begingroup\$ In addition it's very unlikely that one of the purge requests will return 400 because the first step is validate the consumed message (using !agentValidator.validMessage( t )). The idea is that if Varnish is temporarily down we don't want to lose messages (order does not mind). Said that then I should probably make IllegalStateException an IOException since the only feasible error response should be 5XX. Thanks again \$\endgroup\$ – Ga Sacchi Apr 26 '16 at 22:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.